pkp/pkp-lib#11800 Allow manager to edit user setting Appear on the ma…#12471
pkp/pkp-lib#11800 Allow manager to edit user setting Appear on the ma…#12471bozana wants to merge 1 commit intopkp:stable-3_5_0from
Conversation
taslangraham
left a comment
taslangraham
left a comment
There was a problem hiding this comment.
Just a few questions and suggestion, otherwise looks good.
| } | ||
|
|
||
| // Ensure user group exists and belongs to the current context | ||
| $context = $request->attributes->get('context'); /** @var Context $context */ |
There was a problem hiding this comment.
Any reason(or benefits) to getting the Context this way as opposed a more common $this->getRequest()->getContext() pattern?
There was a problem hiding this comment.
I see we now use it this way everywhere in this class
There was a problem hiding this comment.
Added/changed with this commit: 71e79e3#diff-bab50ec5ded0dcc8adbaa27c6ac3428934b6b254145f1340a34be5db63e36eeb
There was a problem hiding this comment.
I would then leave it as it is...
There was a problem hiding this comment.
It seems to be the Laravel HTTP request pipeline, which would already have the context, so I am wondering why we do not use it everywhere... Maybe we should...
There was a problem hiding this comment.
I see it is used in only a handful of other places. But I don't have any strong opinions on whether or not we should adapt it more.
bozana
force-pushed
the
11800-3_5_0
branch
8 times, most recently
from
64c9ba6 to
1433c37
Compare
2 participants
…sthead
s. #11800