Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve type inference for plugin manager implementations #137

Merged
merged 3 commits into from
Jun 13, 2022

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Jun 13, 2022

Q A
QA yes

Description

Improves type inference particularly for plugin managers in dependent libs

@gsteel gsteel changed the title Qa/plugin manager types Improve type inference for plugin manager implementations Jun 13, 2022
@boesing
Copy link
Member

boesing commented Jun 13, 2022

Just a side-note here:

I was about adding this in #98
Back then, @Ocramius told me that this would be a soft BC break due to the way how generics work. So when generic declaration is missing, this will become very messy in projects after upgrading to the version introducing generics.

Therefore, I deferred that to servicemanager v4

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - should we 🚢 ?

src/ServiceLocatorInterface.php Show resolved Hide resolved
src/ServiceManager.php Show resolved Hide resolved
@Ocramius Ocramius added this to the 3.12.0 milestone Jun 13, 2022
@Ocramius Ocramius self-assigned this Jun 13, 2022
@Ocramius
Copy link
Member

Back then, @Ocramius told me that this would be a soft BC break due to the way how generics work.

I think this only applies to our latest CI builds, where psalm would complain about a non-provided template parameter?

I'd say this is good improvement, and doesn't affect the API per se, just documentation. Downstream consumers that use psalm are used to upgrades "shifting" the types a bit, and will adjust.

@boesing
Copy link
Member

boesing commented Jun 13, 2022

I'd say this is good improvement, and doesn't affect the API per se, just documentation. Downstream consumers that use psalm are used to upgrades "shifting" the types a bit, and will adjust.

I'm fine with this.

Anyways: we need to adjust our own components as well, which are quite a lot.
Can we probably create a project for this and list all components which need work then?

Thinking about:

  • input filters
  • validators
  • cache adapters
  • cache plugins (or did I killed that? not 100% sure...)
  • form elements
  • etc.

(btw: with we I mean "not me" 😄)

@Ocramius
Copy link
Member

Can we probably create a project for this and list all components which need work then?

I wonder if the installation of renovate (which didn't happen yet, I know, sorry :( ) would already handle this?

@boesing
Copy link
Member

boesing commented Jun 13, 2022

I wonder if the installation of renovate would already handle this?

Since I don't think that renovate will create a list of components where the update to the latest servicemanager version fails, I'd say no. And I doubt that all the affected projects do have renovate setup anyways.

@Ocramius
Copy link
Member

Well, psalm would fail there, and renovate would open a PR :D

@froschdesign
Copy link
Member

Can we probably create a project for this and list all components which need work then?

I can create the project to track the progress and to give an overview.

@Ocramius
Copy link
Member

@froschdesign please do: I just tried creating one, and it messed up everything, possibly because I have 2 versions of GitHub Projects on my system (the pre-release one too)

@Ocramius
Copy link
Member

Meanwhile, 🚢 here.

@Ocramius Ocramius merged commit db011e3 into laminas:3.12.x Jun 13, 2022
@gsteel gsteel deleted the qa/plugin-manager-types branch June 13, 2022 16:21
@froschdesign
Copy link
Member

@gsteel @Ocramius @boesing
With some delay, here is a first start: https://github.com/orgs/laminas/projects/21

gsteel added a commit to gsteel/laminas-view that referenced this pull request Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants