-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
refactor(Storage): Align all Storage constructors #48614
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: provokateurin <kate@provokateurin.de>
ccf70fc
to
61b97dd
Compare
* @param array $arguments ['storage' => $storage, 'owner' => $owner] | ||
* | ||
* $storage: The storage the permissions mask should be applied on | ||
* $owner: The owner to use in case no owner is found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove comments and type indications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to do this right then it needs proper psalm annotations instead of this.
I don't know if this comment is really helpful in the current state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it explains the expected data format, and it's purpose, so I would say it is useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we also have the problem that the parameters are passed to the parent class.
So if we limit the types using psalm, then a valid input that is only read by the parent class might get rejected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the case here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked, I just wanted to say that it can be a problem.
Summary
Adds strong types, removes wrong doc blocks and uses the same parameter name everywhere.
Checklist