[data_release] refactoring to use data framework. #9051
Conversation
|
@maximemulder would you be able to review / test this? |
| /** | ||
| * Tells the base class that this page's provisioner can support the | ||
| * HasAnyPermissionOrUserSiteMatch filter. | ||
| * | ||
| * @return ?array of site permissions or null | ||
| */ | ||
| public function allSitePermissionNames() : ?array | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I may be mistaken, but this documentation comment does not seem to coincide with the documentation of the base method, no ? It seems to me that returning null means the provisionner does not support the HasAnyPermissionOrUserSiteMatch filter.
Other than that LGTM.
There was a problem hiding this comment.
Hi @maximemulder, thank you for the review. Very appreciated.
This is what I found in the declaration of the method.
The data release docs are not site tied. That's way I'm returning null in the implementation. The page don't use site filtering.
There was a problem hiding this comment.
Do you think it will be more accurate to specify this in the documentation of the implementation itself: That this page don't use site filtering ?
There was a problem hiding this comment.
It might be more accurate, or you could use the {@inheritDoc} annotation
There was a problem hiding this comment.
Do you think it will be more accurate to specify this in the documentation of the implementation itself: That this page don't use site filtering ?
Sorry for the wait, but yes, just like Dave said, specifying either that or inheriting the parent documentation should do the job.
There was a problem hiding this comment.
sorry, somehow this escaped to my attention. Should be fixed now. Thank you.
fixes method documentation.
maximemulder
added
the
Passed manual tests
Brief summary of changes
Refactoring module
Testing instructions (if applicable)
Link(s) to related issue(s)