Skip to content

Feat: Make authorization per-instance - #343

Merged
abnegate merged 14 commits into
feat-framework-v2from
feat-authorization-v2
Jan 25, 2024
Merged

Feat: Make authorization per-instance#343
abnegate merged 14 commits into
feat-framework-v2from
feat-authorization-v2

Conversation

@Meldiron

@Meldiron Meldiron commented Oct 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Makes auth validator instance-aware, to make it work fine with corutines server

  • Update tests

abnegate
abnegate reviewed Oct 24, 2023
View reviewed changes
Comment thread src/Database/Database.php Outdated
Comment thread src/Database/Database.php Outdated
@abnegate
abnegate requested a review from fogelito October 24, 2023 07:08
fogelito
fogelito reviewed Oct 24, 2023
View reviewed changes
Comment thread src/Database/Adapter/MariaDB.php Outdated
fogelito
fogelito reviewed Oct 24, 2023
View reviewed changes
Comment thread src/Database/Database.php
@github-actions github-actions Bot mentioned this pull request Nov 1, 2023
Closed
@Meldiron
Meldiron marked this pull request as ready for review January 3, 2024 13:17
@Meldiron Meldiron mentioned this pull request Jan 3, 2024
Merged
abnegate
abnegate reviewed Jan 8, 2024
View reviewed changes

@abnegate abnegate left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can't comment on the Base.php diff since it's so big 😅 but do we need both the static and instance Authorization instances there?

Comment thread src/Database/Validator/Authorization.php Outdated
Comment thread src/Database/Database.php Outdated
Comment thread src/Database/Validator/Authorization.php
Comment thread src/Database/Validator/Authorization.php Outdated
Comment thread src/Database/Validator/Authorization.php
@Meldiron

Meldiron commented Jan 8, 2024

Copy link
Copy Markdown
Contributor Author

@abnegate

but do we need both the static and instance Authorization instances there?

Authorization:: is no more. You always must new Authroization(). No backwards compatibility. Breaking change.

@abnegate

abnegate commented Jan 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

@abnegate

but do we need both the static and instance Authorization instances there?

Authorization:: is no more. You always must new Authroization(). No backwards compatibility. Breaking change.

@Meldiron I mean in the test base class we have an instance var authorization and a static var authorizationSingleton, I think we only need one or the other

@Meldiron

Meldiron commented Jan 24, 2024

Copy link
Copy Markdown
Contributor Author

authorizationSingleton

I cant improve this without renaming var in ~100 lines. The setup that is there still make sense - it's not Utopia-spepcfiic, but PHPUnit-specific.
static-var is there so state of it is shared across all tests.
instnace-var is there so we don't need to call static method in all tests.

abnegate
abnegate reviewed Jan 25, 2024
View reviewed changes
Comment thread src/Database/Database.php Outdated
Comment thread src/Database/Database.php Outdated
Comment thread src/Database/Validator/Authorization.php
Comment thread tests/Database/Base.php Outdated
abnegate
abnegate reviewed Jan 25, 2024
View reviewed changes
Comment thread src/Database/Validator/Authorization.php Outdated
@abnegate
abnegate merged commit b64caf4 into feat-framework-v2 Jan 25, 2024
@abnegate
abnegate deleted the feat-authorization-v2 branch January 25, 2024 11:30
@github-actions github-actions Bot mentioned this pull request Feb 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abnegate abnegate abnegate approved these changes
@fogelito fogelito fogelito approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Meldiron @abnegate @fogelito