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

Update laminas dependencies #102

Open
wants to merge 14 commits into
base: 2.23.x
Choose a base branch
from

Conversation

stanis-molnar
Copy link

@stanis-molnar stanis-molnar commented Jan 22, 2025

This PR simply updates the laminas dependencies and adapts accordingly the use of the new major versions of laminas-servicemanager and laminas-validator.

I also removed the php annotations and added explicit return types to the modified classes.

This unfortunately breaks backwards compatibility and will require the release of a new major version of laminas-session.

@stanis-molnar stanis-molnar force-pushed the update-laminas-dependencies branch 2 times, most recently from f4c1f0a to 3c0ed84 Compare January 23, 2025 08:07
stanis-molnar and others added 13 commits January 23, 2025 15:46
* AbstractFactoryInterface was moved to the factory subfolder
* Using Psr\Container\ContainerInterface instead of Interop\Container\ContainerInterface
* Adding return types

Signed-off-by: Daniel Molnar <[email protected]>
…onTrait::getListenersForEvent

The Session ManagerInterface defines `getValidatorChain()` to return an `EventManagerInterface`, but `EventListenerIntrospectionTrait::getListenersForEvent()` expects an `EventManager` instance.
For purposes of the tests, the returned value _is_ an EM instance, so the error is invalid.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Since the v2 `createService()` method cannot be removed before v4, we need to tell Psalm that (a) the method needs to exist, and (b) the second argument is required even if it's not used.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Since the v2 `createService()` method cannot be removed before v4, we need to tell Psalm that (a) the method needs to exist, and (b) the second argument is required even if it's not used.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Since the v2 `createService()` method cannot be removed before v4, we need to tell Psalm that (a) the method needs to exist, and (b) the second argument is required even if it's not used.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
- Made each of $config and $sessionManager props nullable, defaulting to null, as there is no constructor whereby they will be set by default.
  - Changed `isset()` calls on each property to check type of property instead.
- Suppress PossiblyUnusedParam and PossiblyUnusedMethod warnings for v2 methods.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
- Specify `$id` arguments as non-empty-string values
- Suppress RedundantCast, as the parent interface may not have the return types defined correctly in all versions of PHP

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Signed-off-by: Matthew Weier O'Phinney <[email protected]>
@weierophinney weierophinney force-pushed the update-laminas-dependencies branch from 1ed3008 to 04c64db Compare January 23, 2025 21:49
@weierophinney weierophinney added this to the 2.23.0 milestone Jan 23, 2025
@weierophinney weierophinney added the Dependencies Updates and changes to dependencies label Jan 23, 2025
Signed-off-by: Matthew Weier O'Phinney <[email protected]>
@weierophinney weierophinney modified the milestones: 2.23.0, 3.0.0 Jan 23, 2025
@weierophinney
Copy link
Member

weierophinney commented Jan 23, 2025

@stanis-molnar — I've fixed the Psalm issues that were reported and pushed back to your branch.

I missed the part where you indicated a new major would be needed. I'll check with the @laminas/technical-steering-committee to see how we clear the BC breaks, and verify everyone is okay with the new major.

@stanis-molnar
Copy link
Author

stanis-molnar commented Jan 24, 2025

@stanis-molnar — I've fixed the Psalm issues that were reported and pushed back to your branch.

I missed the part where you indicated a new major would be needed. I'll check with the @laminas/technical-steering-committee to see how we clear the BC breaks, and verify everyone is okay with the new major.

@weierophinney Thanks very much for taking care of that! Please let me know if any further actions on my side are needed.

@arhimede
Copy link
Member

arhimede commented Jan 24, 2025

@weierophinney
we are about to do 2 different PRs':

  • one against V2 to mark all deprecated functions as deprecated and to have new V2 release with that
  • one for V3 in order to implement service manager v4 and remove all deprecated functions

Now with this PR we are confused if is the case or not to commit our PR's, since is a different approach :-)
Not sure why you say "Since the v2 createService() method cannot be removed before v4" , not sure what is the roadmap for this library .

@weierophinney
Copy link
Member

@weierophinney we are about to do 2 different PRs':

  • one against V2 to mark all deprecated functions as deprecated and to have new V2 release with that
  • one for V3 in order to implement service manager v4 and remove all deprecated functions

Now with this PR we are confused if is the case or not to commit our PR's, since is a different approach :-) Not sure why you say "Since the v2 createService() method cannot be removed before v4" , not sure what is the roadmap for this library .

I wasn't aware you were working on one, @arhimede ; I reviewed this one initially as it looked like it might fit in the 2.23.0 milestone. When I wrote those commit messages, I'd forgotten that this was the v2 series; that should have read "before v3".

I'm fine with waiting for your PR; marking deprecations first and then doing removal for next major makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dependencies Updates and changes to dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants