Upgrade Service Manager to v4 in `require`, Cache to v4 and Validator to v3 in `require-dev` #103

Jurj-Bogdan · 2025-01-24T10:41:46Z

Upgraded laminas/laminas-servicemanager to version 4, as well as the other dependencies required for it, and added various Psalm tweaks.

I seems I was working on this PR alongside #102 , with the main difference seemingly being the removal of v2 functions from the codebase in preparation for a new major release.

I was about to make a different PR to the current branch marking the functions removed here as deprecated, but I'll wait for feedback regarding the desired way forward.

arhimede · 2025-01-24T15:23:15Z

@Jurj-Bogdan See #102
Please open a separate PR, which do not break BC , and against 2.23 branch, for the milestone 2.23.1

arhimede · 2025-02-04T11:45:25Z

@laminas/technical-steering-committee
We have this PR and the other PR, #102
which are doing the same thing.
Only that this PR is removing the deprecated function .

So, should I create the branch 3.0.x and merge this one there ?

gsteel · 2025-02-04T11:59:44Z

So, should I create the branch 3.0.x and merge this one there

Yes, create 3.0.x from the tip of the next minor release - You should probably get 2.24.x patches resolved first such as #106 so that merge-ups from 2.x have fewer conflicts

arhimede · 2025-02-05T11:00:34Z

@Jurj-Bogdan Please fix the conflicts

src/SaveHandler/Cache.php

gsteel · 2025-02-07T12:15:51Z

src/Service/ContainerAbstractServiceFactory.php

@@ -41,28 +40,20 @@ class ContainerAbstractServiceFactory implements AbstractFactoryInterface
 {


This class should be made final and all properties private. You should also mark this class soft final with @final in a 2.x release

Done - also a related question here. I just noticed there are already some classes marked as deprecated, maybe this should be a good time to remove them as well?

Talking about AbstractValidatorChain and AbstractValidatorChainEM3

Yes - anything that's deprecated should be dealt with in the next major. I don't know this lib well enough to know what the implications or plans might have been though.

Don't forget to document any BC breaks here with companion patches against 2.x including deprecations, and, make a start on a migration guide that lists all the BC breaking changes

src/Service/ContainerAbstractServiceFactory.php

src/Validator/Csrf.php

gsteel · 2025-02-07T12:28:27Z

src/Validator/Csrf.php

- *     session?: Container,
- *     timeout?: ?int,
- * }
- */
 final class Csrf extends AbstractValidator


This whole class will probably need further refactoring such as deprecating and dropping all the public setters and getters

Not suggesting that work should happen in this patch tho!

gsteel · 2025-02-07T12:35:50Z

test/Service/SessionManagerFactoryTest.php

@@ -114,7 +114,8 @@ public function testFactoryWillAddValidatorViaConfiguration(): void

        $manager->start();

-        $chain     = $manager->getValidatorChain();
+        $chain = $manager->getValidatorChain();
+        /** @psalm-suppress ArgumentTypeCoercion **/


Please remove this suppression and replace it with assertions.

Assert the manager is ManagerInterface
Assert the chain is a EventManagerInterface or whatever it's supposed to be

Added some phpunit assertions as they're useful, but Psalm is still adamant in throwing "ArgumentTypeCoercion" errors which is why the suppressions are still there.

You need to assert $chain is an instance of EventManager (The concrete class) because that's what getListenersForEvent expects.

Please also add findUnusedPsalmSuppress="true" to psalm.xml so that unused inline suppressions are flagged.

gsteel · 2025-02-07T12:36:05Z

test/Service/SessionManagerFactoryTest.php

@@ -198,7 +199,8 @@ public function testFactoryDoesNotAttachValidatorTwoTimes(): void
            // Ignore exception, because we are not interested whether session validation passes in this test
        }

-        $chain     = $manager->getValidatorChain();
+        $chain = $manager->getValidatorChain();
+        /** @psalm-suppress ArgumentTypeCoercion **/


Same here… More assertions!

This suppression still needs fixing too!

psalm-baseline.xml

Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

gsteel

Thanks @Jurj-Bogdan - A few more tweaks please :)

Apart from inline comments, all of the changed factories need to be made final and they should all have a good cleanup - i.e protected props to private, drop any methods that are pointless etc. Happy to wait for another patch for those things, but we really don't want users extending factories and need to prevent that in the next major

gsteel · 2025-02-18T16:36:38Z

src/Service/ContainerAbstractServiceFactory.php

     */
-    protected function getConfig(ContainerInterface $container)
+    protected function getConfig(ContainerInterface $container): array


This should be private

gsteel · 2025-02-18T16:36:44Z

src/Service/ContainerAbstractServiceFactory.php

     */
-    protected function getSessionManager(ContainerInterface $container)
+    protected function getSessionManager(ContainerInterface $container): ?ManagerInterface


This should be private

gsteel · 2025-02-18T16:37:18Z

src/Service/ContainerAbstractServiceFactory.php

     */
-    protected function normalizeContainerName($name)
+    protected function normalizeContainerName(string $name): string


This should be private or maybe inlined if it's only used once

gsteel · 2025-02-18T16:44:45Z

test/Service/SessionManagerFactoryTest.php

@@ -114,7 +114,8 @@ public function testFactoryWillAddValidatorViaConfiguration(): void

        $manager->start();

-        $chain     = $manager->getValidatorChain();
+        $chain = $manager->getValidatorChain();
+        /** @psalm-suppress ArgumentTypeCoercion **/


You need to assert $chain is an instance of EventManager (The concrete class) because that's what getListenersForEvent expects.

Please also add findUnusedPsalmSuppress="true" to psalm.xml so that unused inline suppressions are flagged.

gsteel · 2025-02-18T16:53:37Z

test/Service/SessionManagerFactoryTest.php

@@ -198,7 +199,8 @@ public function testFactoryDoesNotAttachValidatorTwoTimes(): void
            // Ignore exception, because we are not interested whether session validation passes in this test
        }

-        $chain     = $manager->getValidatorChain();
+        $chain = $manager->getValidatorChain();
+        /** @psalm-suppress ArgumentTypeCoercion **/


This suppression still needs fixing too!

Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

…ed deprecated usage functions Signed-off-by: Jurj-Bogdan <[email protected]>

Signed-off-by: Jurj-Bogdan <[email protected]>

…ed deprecated usage functions Signed-off-by: Jurj-Bogdan <[email protected]>

Signed-off-by: Jurj-Bogdan <[email protected]>

…hanges Signed-off-by: Jurj-Bogdan <[email protected]>

Jurj-Bogdan · 2025-03-11T14:26:40Z

@gsteel Just added the latest requested changes, i hope i didn't miss anything. A note related to the Csrf.php changes however:

My understanding of the comment is that Csrf objects should only be initialised via constructor, with all properties being immutable - is this correct?

Since the old getters for both hash and session set values for each of them in case they were null, i went ahead and made them not nullable in the first place, although the the changes in the constructor might be a bit ugly.

Also removed the sequences marked as "@todo remove", including the adding of the hash to the session container directly instead of as an entry in the "tokenList" only - i take it this feature is no longer wanted, right?

As for the failing test, since issue #115 mentions removing mongoDb altogether, I'll go ahead and create a different PR with that change (and for the other sub issues as well) so this PR doesn't get too cluttered.

gsteel · 2025-03-11T15:49:07Z

If all the red CI here is MongoDB's fault, why not work on a patch to fix #115 first and then rebase here when its done.

gsteel · 2025-03-11T15:51:22Z

Mind you, looking at the CI logs, the problem is a simple deprecation in the mongo lib. Has anyone looked at just fixing that deprecation?

Jurj-Bogdan · 2025-03-11T16:00:48Z

Mind you, looking at the CI logs, the problem is a simple deprecation in the mongo lib. Has anyone looked at just fixing that deprecation?

That would be the fix in another PR for the 2.x version, but from what i understand mongoDb is to be removed in version 3, no?

gsteel · 2025-03-11T16:15:50Z

That would be the fix in another PR for the 2.x version, but from what i understand mongoDb is to be removed in version 3, no?

There's currently no rationale for its removal as far as I know - just an issue to remove it.

Perhaps @arhimede has more information on why it should be removed

arhimede · 2025-03-14T10:16:25Z

That would be the fix in another PR for the 2.x version, but from what i understand mongoDb is to be removed in version 3, no?

There's currently no rationale for its removal as far as I know - just an issue to remove it.

Perhaps @arhimede has more information on why it should be removed

First of all, MongoDB is an external dependency of this package , I see no point to be included in this package .
When there is a need , a special laminas-session-mongodb package can be created .
I totally agree with your comment here : #115 (comment)

Last but not least, see this old discussion on this matter: #23

arhimede · 2025-03-14T10:19:55Z

And not to mention the licensing of MongoDB which is quite complex.

gsteel · 2025-03-14T10:52:22Z

I have no problem with it being removed as long as the decision making and docs are sound 👍

My point here is: Why not send in a patches now to…

Deprecate mongo features in 2.x
Delete them in 3.x

That way devlopment can move forward on other things more easily because there will be fewer red builds

Jurj-Bogdan mentioned this pull request Jan 24, 2025

marked methods to be removed in version 3 as deprecated, updated composer lock #104

Merged

arhimede linked an issue Jan 28, 2025 that may be closed by this pull request

Version 3 + Implement service manager 4 #101

Open

arhimede added this to the 3.0.0 milestone Feb 5, 2025

arhimede changed the base branch from 2.24.x to 3.0.x February 5, 2025 10:58

arhimede requested a review from gsteel February 6, 2025 21:08

gsteel requested changes Feb 7, 2025

View reviewed changes

gsteel added Enhancement Dependencies Updates and changes to dependencies labels Feb 7, 2025

renovate bot added 2 commits February 10, 2025 01:08

Lock file maintenance

1a26151

Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Lock file maintenance

290f853

Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

arhimede requested a review from gsteel February 17, 2025 10:18

gsteel requested changes Feb 18, 2025

View reviewed changes

gsteel added the BC Break label Feb 18, 2025

gsteel changed the title ~~bumped laminas-servicemanager to v4 alongside its dependencies, remov…~~ Upgrade Service Manager to v4 in require, Cache to v4 and Validator to v3 in require-dev Feb 18, 2025

renovate bot and others added 8 commits February 24, 2025 03:11

Lock file maintenance

cf38ed3

Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

bumped laminas-servicemanager to v4 alongside its dependencies, remov…

f229316

…ed deprecated usage functions Signed-off-by: Jurj-Bogdan <[email protected]>

added missing import

e450d39

Signed-off-by: Jurj-Bogdan <[email protected]>

psalm baseline update

5e66a5c

Signed-off-by: Jurj-Bogdan <[email protected]>

bumped laminas-servicemanager to v4 alongside its dependencies, remov…

0ab863a

…ed deprecated usage functions Signed-off-by: Jurj-Bogdan <[email protected]>

branch rebase, conflicts resolved

3735510

Signed-off-by: Jurj-Bogdan <[email protected]>

requested changes

690f522

Signed-off-by: Jurj-Bogdan <[email protected]>

Csrf refactoring, factories marked as final & other minor requested c…

61f4a8c

…hanges Signed-off-by: Jurj-Bogdan <[email protected]>

Jurj-Bogdan force-pushed the laminas-servicemanager-4 branch from 3411dbd to 61f4a8c Compare March 11, 2025 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Service Manager to v4 in `require`, Cache to v4 and Validator to v3 in `require-dev` #103

Upgrade Service Manager to v4 in `require`, Cache to v4 and Validator to v3 in `require-dev` #103

Jurj-Bogdan commented Jan 24, 2025

arhimede commented Jan 24, 2025

arhimede commented Feb 4, 2025

gsteel commented Feb 4, 2025

arhimede commented Feb 5, 2025

gsteel Feb 7, 2025

Jurj-Bogdan Feb 13, 2025

gsteel Feb 18, 2025

gsteel Feb 7, 2025

gsteel Feb 7, 2025

gsteel Feb 7, 2025

Jurj-Bogdan Feb 13, 2025

gsteel Feb 18, 2025

gsteel Feb 7, 2025

gsteel Feb 18, 2025

gsteel left a comment

gsteel Feb 18, 2025

gsteel Feb 18, 2025

gsteel Feb 18, 2025

gsteel Feb 18, 2025

gsteel Feb 18, 2025

Jurj-Bogdan commented Mar 11, 2025 •

edited

Loading

gsteel commented Mar 11, 2025

gsteel commented Mar 11, 2025

Jurj-Bogdan commented Mar 11, 2025

gsteel commented Mar 11, 2025

arhimede commented Mar 14, 2025

arhimede commented Mar 14, 2025

gsteel commented Mar 14, 2025

		@@ -41,28 +40,20 @@ class ContainerAbstractServiceFactory implements AbstractFactoryInterface
		{

Upgrade Service Manager to v4 in require, Cache to v4 and Validator to v3 in require-dev #103

Are you sure you want to change the base?

Upgrade Service Manager to v4 in require, Cache to v4 and Validator to v3 in require-dev #103

Conversation

Jurj-Bogdan commented Jan 24, 2025

arhimede commented Jan 24, 2025

arhimede commented Feb 4, 2025

gsteel commented Feb 4, 2025

arhimede commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsteel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jurj-Bogdan commented Mar 11, 2025 • edited Loading

gsteel commented Mar 11, 2025

gsteel commented Mar 11, 2025

Jurj-Bogdan commented Mar 11, 2025

gsteel commented Mar 11, 2025

arhimede commented Mar 14, 2025

arhimede commented Mar 14, 2025

gsteel commented Mar 14, 2025

Upgrade Service Manager to v4 in `require`, Cache to v4 and Validator to v3 in `require-dev` #103

Upgrade Service Manager to v4 in `require`, Cache to v4 and Validator to v3 in `require-dev` #103

Jurj-Bogdan commented Mar 11, 2025 •

edited

Loading