pkp/pkp-lib/#10571 email access restrictions#11090
Conversation
taslangraham
force-pushed
the
feature-i10403-email-access-restrictions
branch
3 times, most recently
from
d4b1aa5 to
e52d322
Compare
| throw new Exception('Invalid value given for the `isUnrestricted` attribute on the ' . $attrs['key'] . ' template.'); | ||
| } | ||
|
|
||
| $contextIds = app()->get('context')->getIds(); |
There was a problem hiding this comment.
Is there a reason to use the app() helper instead of one of the built in PKP methods to get all context IDs? Would a method from Application::get()->getContextDao() work?
There was a problem hiding this comment.
The closest thing to getIds that is available via Application::get()->getContextDao() is a getAll method, but that returns a DAOResultFactory with objects for all context which you'd then have to process to get all IDs. However, going through the code, I saw that app()->get('context')->getIds() was routinely used to get all IDs so I went with that appraoch.
There was a problem hiding this comment.
Right, that makes sense. And I don't recall the context now, but I remember loading the entirety of all contexts at once had performance implications on larger installs, so this all sounds good.
| ->whereNot('user_group_id', null) | ||
| ->get() | ||
| ->pluck('userGroupId') | ||
| ->all(); |
There was a problem hiding this comment.
Also wondering about the difference between using toArray() vs. all() here and the use of pluck above.
There was a problem hiding this comment.
Given that we operating on a simple result from usage of pluck, I don't think there's any real difference. For consistency I've updated both places to use the all
There was a problem hiding this comment.
Perfect. Didn't imagine there was a huge difference, but good to settle on a consistent way of doing it.
…il templates within a mailable
taslangraham
force-pushed
the
feature-i10403-email-access-restrictions
branch
from
e52d322 to
7e33786
Compare
2 participants
For #10571