Fix for #3039636 #129
Fix for #3039636 #129
Conversation
…e-load all the schemes every time
| @@ -172,12 +172,6 @@ public function load($id) { | |||
| * {@inheritdoc} | |||
| */ | |||
| public function checkEntityAccess(AccessSchemeInterface $scheme, EntityInterface $entity, $op, AccountInterface $account) { | |||
There was a problem hiding this comment.
Are we certain this code isn't called anywhere else?
| @@ -63,7 +63,11 @@ function workbench_access_form_alter(&$form, FormStateInterface $form_state) { | |||
| function workbench_access_entity_access(EntityInterface $entity, $op, AccountInterface $account) { | |||
| // Return net result of all enabled access schemes. If one scheme allows | |||
| // access, then it is granted. | |||
| $manager = \Drupal::service('plugin.manager.workbench_access.scheme'); | |||
There was a problem hiding this comment.
Part of the point to the service here is to enable issues like #122.
IS loading the service really that much of a performance hit?
There was a problem hiding this comment.
phpstorm is reporting $manager as unused, so I removed it. tests pass so I think its safe to remove the variable
There was a problem hiding this comment.
Right, but is it called by any other code? We're changing the behavior of the service....
I'm on the road and can look more deeply tomorrow.
|
I'll put the line back,but it is dead code
…On Fri, Mar 15, 2019, 12:38 AM Ken Rickard ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In workbench_access.module
<#129 (comment)>
:
> @@ -63,7 +63,11 @@ function workbench_access_form_alter(&$form, FormStateInterface $form_state) {
function workbench_access_entity_access(EntityInterface $entity, $op, AccountInterface $account) {
// Return net result of all enabled access schemes. If one scheme allows
// access, then it is granted.
- $manager = \Drupal::service('plugin.manager.workbench_access.scheme');
Right, but is it called by any other code? We're changing the behavior of
the service....
I'm on the road and can look more deeply tomorrow.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#129 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAh49iY_X6lNyc14aW_4zuXDklIcmdCkks5vWl7ngaJpZM4bsXkz>
.
|
|
Put the line back
On Fri, 15 Mar 2019 at 05:50, Lee Rowlands <[email protected]>
wrote:
… I'll put the line back,but it is dead code
On Fri, Mar 15, 2019, 12:38 AM Ken Rickard ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In workbench_access.module
> <#129 (comment)>
> :
>
> > @@ -63,7 +63,11 @@ function workbench_access_form_alter(&$form, FormStateInterface $form_state) {
> function workbench_access_entity_access(EntityInterface $entity, $op, AccountInterface $account) {
> // Return net result of all enabled access schemes. If one scheme allows
> // access, then it is granted.
> - $manager = \Drupal::service('plugin.manager.workbench_access.scheme');
>
> Right, but is it called by any other code? We're changing the behavior of
> the service....
>
> I'm on the road and can look more deeply tomorrow.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#129 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAh49iY_X6lNyc14aW_4zuXDklIcmdCkks5vWl7ngaJpZM4bsXkz>
> .
>
--
Lee Rowlands
Senior Drupal Developer
PreviousNext
email: [email protected]
|
|
@agentrickard any updates on this, big performance boost |
|
No updates. I'm totally swamped. My issue here is that I think the 'view' $op check belongs in the method because that class is a service, and the method should be opinionated. I think we can have the logic in both places for performance reasons, but I wouldn't move the $op check. |
| if ($op === 'view' || $op === 'view label' || $account->hasPermission('bypass workbench access')) { | ||
| // Return early. | ||
| return AccessResult::neutral(); | ||
| } | ||
| return array_reduce(\Drupal::entityTypeManager()->getStorage('access_scheme')->loadMultiple(), function (AccessResult $carry, AccessSchemeInterface $scheme) use ($entity, $op, $account) { |
There was a problem hiding this comment.
I think I have totally misread this patch because the anonymous function here calls the $manager directly. I kept misreading that we were loading checkEntityAccess() from a service and not a base class.
This is one reason why I despise anonymous functions. Every time we use array_reduce in this code, I can't follow the logic easily.
https://www.drupal.org/project/workbench_access/issues/3039636
Profiling result of applying the patch/fix