feat: migrate from deprecated PublicEmitter to IEventDispatcher

[bdovaz]

Fixes https://github.com/nextcloud/user_saml/issues/853

[bdovaz]

@blizzz I have doubts with these lines:

https://github.com/nextcloud/user_saml/blob/edb21705e2963322d38e0323baccd9bad69e0976/tests/stub.phpstub#L29

Should I remove them? I don't really know what this file is for, I come from the .NET world. 😅

[blizzz]

@blizzz I have doubts with these lines:

https://github.com/nextcloud/user_saml/blob/edb21705e2963322d38e0323baccd9bad69e0976/tests/stub.phpstub#L29

Should I remove them? I don't really know what this file is for, I come from the .NET world. 😅

So that's for testing, and the PublicEmitter is a private interface from the server, so it was stubbed here. By dropping that interface from lib/GroupManager.php we may do not need the stub anymore, and the entry can be removed just as well.

[bdovaz]

@blizzz I have doubts with these lines: https://github.com/nextcloud/user_saml/blob/edb21705e2963322d38e0323baccd9bad69e0976/tests/stub.phpstub#L29

Should I remove them? I don't really know what this file is for, I come from the .NET world. 😅

So that's for testing, and the PublicEmitter is a private interface from the server, so it was stubbed here. By dropping that interface from lib/GroupManager.php we may do not need the stub anymore, and the entry can be removed just as well.

Removed! PR ready to review!

[github-actions[bot]]

Hello there, Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

[blizzz]

There is failing CI. I see this also in an unrelated PR, so it do not think it is related to your changes. Nevertheless, I want to have this sorted out first so we have a green CI by default :)

[blizzz]

There is failing CI. I see this also in an unrelated PR, so it do not think it is related to your changes. Nevertheless, I want to have this sorted out first so we have a green CI by default :)

It was a regression within server, but fixed now.

[bdovaz]

@blizzz fixed php lint issues, rerun please!

[blizzz]

Now unit tests, they need adjustements. The IEventDispatcher has to be passed here https://github.com/nextcloud/user_saml/blob/master/tests/unit/GroupManagerTest.php#L57-L58 and maybe we want to check that dispatchTyped() is called with the correct event class.

[bdovaz]

@blizzz done! Please rerun, thanks!

Is there any way to run the tests locally so that I don't depend on you to approve the workflow execution? Because looking at this file it seems that it depends on the fact that they must be executed through the workflow:

https://github.com/nextcloud/user_saml/blob/1a180957787d140af00d861847cc159cd84c0439/tests/unit/bootstrap.php#L10

[blizzz]

If you have user_saml in an apps folder in a server checkout running locally will work. composer i && composer run test:unit should be sufficient then.

[bdovaz]

@blizzz now I hope that everything is corrected, locally at least everything works correctly.

My problem was that I did not understand well the PHPUnit API and expects method when we want to check multiple invocations with different parameters of a method.

In my case I have not used the "at" method that is used in "UserBackendTest.php" because it says that it is deprecated and will be removed in the next version of PHPUnit. I could correct the one in "UserBackendTest.php" but I don't want to do it in this PR to avoid touching files that I should not.

[bdovaz]

@blizzz I don't understand the errors that appear in the MySQL workflow, if the tests are exactly the same and in other database providers they have worked correctly (SQLite and PostgreSQL) how is it possible that in MySQL not and with this error?

1) OCA\User_SAML\Tests\GroupManagerTest::testUpdateUserGroups
Creation of dynamic property OCA\User_SAML\Tests\GroupManagerTest::$eventDispatcher is deprecated

/home/runner/actions-runner/_work/user_saml/user_saml/apps/user_saml/tests/unit/GroupManagerTest.php:50

https://github.com/nextcloud/user_saml/actions/runs/9718183419/job/26826566029?pr=856

[bdovaz]

@blizzz I have corrected it anyway, it is that the field at class level was missing. The strange thing as I say is that it fails in some workflows but not in others. Apart from the fact that it is a warning but in reality it considers it an error.

You can run the build again.

[blizzz]

@blizzz I have corrected it anyway, it is that the field at class level was missing. The strange thing as I say is that it fails in some workflows but not in others. Apart from the fact that it is a warning but in reality it considers it an error.

You can run the build again.

Thanks! Dynamic properties were deprecated in PHP 8.2, so earlier ones would pass.

[bdovaz]

@blizzz have passed all the checks!

Merge whenever you want, I'm all set!

Thanks.

[blizzz]

Great! Thank you @bdovaz !