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.

Sign up for GitHub

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

Jump to bottom

Make the container fully fulfill PSR container interface #36417

Merged
merged 2 commits into from
Feb 7, 2023
Merged

Make the container fully fulfill PSR container interface #36417

merged 2 commits into from
Feb 7, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 28, 2023 •
edited
Loading

Summary

To fulfill the PSR container interface it must throw NotFoundExceptionInterface if the requested name was not found within the container.
Added a simple private wrapper over the QueryException implementing the NotFoundExceptionInterface which is only thrown if the query was not found, this way the public API should be unchanged and only the thrown error additionally implements NotFoundExceptionInterface.

Checklist

@susnux susnux added bug 3. to review Waiting for reviews php Pull requests that update Php code labels Jan 28, 2023
@susnux susnux force-pushed the fix/psr-container branch from bdc3e43 to dbcd9bc Compare January 28, 2023 05:18
@szaimen szaimen added this to the Nextcloud 26 milestone Jan 28, 2023
@nickvergessen nickvergessen requested review from a team, ArtificialOwl, icewind1991 and blizzz and removed request for a team January 30, 2023 08:24
nickvergessen
nickvergessen approved these changes Jan 30, 2023
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

https://www.php-fig.org/psr/psr-11/#33-psrcontainernotfoundexceptioninterface

ChristophWurst
ChristophWurst approved these changes Jan 30, 2023
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Thanks!

@ChristophWurst
Copy link
Member

https://github.com/nextcloud/3rdparty/blob/1d53ed4d3282427854fca7ee6ecbb945304272db/composer.json#L47 time to bump that to v2

@susnux
Copy link
Contributor Author

susnux commented Jan 30, 2023 •
edited
Loading

time to bump that to v2

Would include extending Throwable to the exceptions and adding the boolean return type hint to the has method.
As far as I can see this is already done on our implementation (extending Exception which implements Throwable), so this should be no breaking change.
Should I include this within this PR?

And I do not know the server backport policies, but I would highly appreciate to have this in NC25.

@susnux susnux force-pushed the fix/psr-container branch from dbcd9bc to aadc0ba Compare January 31, 2023 16:39
@ChristophWurst
Copy link
Member

Let's only fix the PSR compliance here to keep this backportable and do the bump for master/26 only.

@blizzz blizzz mentioned this pull request Feb 1, 2023
Merged
@susnux
Copy link
Contributor Author

susnux commented Feb 2, 2023

I have no clue why drone is failing, but it seems unrelated

@ChristophWurst
Copy link
Member

Test\AppFramework\Utility\SimpleContainerTest::testNothingRegistered sounds related

@susnux susnux force-pushed the fix/psr-container branch from aadc0ba to 3310629 Compare February 2, 2023 10:35
susnux
susnux commented Feb 2, 2023
View reviewed changes
lib/public/AppFramework/QueryException.php
@@ -32,7 +32,7 @@
/**
* Class QueryException
*
* The class extends `NotFoundExceptionInterface` since 20.0.0
* The class extends `ContainerExceptionInterface` since 20.0.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noticed this commend was wrong, as the class did not extend the not found interface but the container exception interface. Fixed.

@susnux susnux force-pushed the fix/psr-container branch from 3310629 to 6465d73 Compare February 2, 2023 13:42
@susnux susnux force-pushed the fix/psr-container branch from 6465d73 to 2f18180 Compare February 2, 2023 21:47
@susnux susnux requested a review from miaulalala as a code owner February 2, 2023 21:47
@susnux
Copy link
Contributor Author

susnux commented Feb 2, 2023 •
edited
Loading

I fixed that issue, but that CI still fails with some issues on the encryption, which seem definitely unrelated, but I rebased onto current master to hopefully fix that drone nodb check.
Well the remaining drone issues seem to be unrelated, as the same issues are reported for every PR currently ...

@nickvergessen
Copy link
Member

There was 1 failure:

  1. Test\AppFramework\Utility\SimpleContainerTest::testNothingRegistered
    Failed asserting
    that OCP\AppFramework\QueryException Object (...) is an instance of
    interface "Psr\Container\NotFoundExceptionInterface".

/drone/src/tests/lib/AppFramework/Utility/SimpleContainerTest.php:87

@nickvergessen
Copy link
Member

For the encryption error just rebase, the above error is your code change and needs fixing/adjusting

@susnux susnux force-pushed the fix/psr-container branch 2 times, most recently from a9962e1 to 795370d Compare February 6, 2023 12:46
@susnux
fix: Throw NotFoundExceptionInterface to fulfill PSR container inte…
ba8a50c 
…rface if class not found

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the fix/psr-container branch from 795370d to 0e83576 Compare February 6, 2023 13:16
@susnux
Copy link
Contributor Author

susnux commented Feb 6, 2023

@nickvergessen thank you! I forgot to handle querying with autoloading enabled, this is fixed now and the test passes.

@susnux
Update autoloaders
3b2d01f 
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the fix/psr-container branch from 0e83576 to 3b2d01f Compare February 7, 2023 16:50
@nickvergessen
Copy link
Member

Something to document in #34692 ?

@nickvergessen nickvergessen merged commit dcfc96f into master Feb 7, 2023
@nickvergessen nickvergessen deleted the fix/psr-container branch February 7, 2023 19:38
@susnux
Copy link
Contributor Author

susnux commented Feb 8, 2023

/backport to stable25

@susnux susnux mentioned this pull request Feb 8, 2023
Merged
@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

@susnux susnux mentioned this pull request Feb 8, 2023
Merged
3 tasks
@blizzz blizzz mentioned this pull request Feb 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@nickvergessen nickvergessen nickvergessen approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 was automatically assigned from nextcloud/server-backend

@blizzz blizzz Awaiting requested review from blizzz blizzz was automatically assigned from nextcloud/server-backend

@miaulalala miaulalala Awaiting requested review from miaulalala

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug php Pull requests that update Php code
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.
5 participants
@susnux @ChristophWurst @nickvergessen @skjnldsv @szaimen