Here is some feedback from https://www.drupal.org/node/2694755#comment-11009515

FlagAccessController isn't really a controller, just has two access methods that are re-used. This would be better as a registered access check. See NodeAddAccessCheck for example and how it is registered in node.services.yml. You could register it as _flag_access, with the value flag/unflag. That's a lot easier to use in the various routes.

That sounds like a really good idea.

I am just spinning off a new issue to deal with that.

CommentFileSizeAuthor
#12 interdiff-2695983-3-12.txt3.09 KBmartin107
#12 accessCheck-2695983-12.patch6.39 KBmartin107
#3 accessCheck-2695983-3.patch5.99 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107 created an issue. See original summary.

joachim’s picture

joachim CreditAttribution: joachim commented

Yup, this sounds good.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: martin107 » Unassigned
Status: Active » Needs review
FileSize
accessCheck-2695983-3.patch5.99 KB

This removes one class from \Drupal\flag namespace - which is a good thing.

I have tested it manually and can confirm a simple flag can be flagged and unflagged. I expect testbot may still find problems.

Regarding phpcs, both new classes are clean.

As a small note

We are copying from NodeAddAccessCheck which implements \Drupal\Core\Routing\Access\AccessInterface

AccessInterface is an empty Interface which this #2266817: Deprecate empty AccessInterface and remove usages proposes to mark it

@deprecated in Drupal 8.x-dev, will be removed before Drupal 9.0.

For the moment we must also extend it.. but down the road there will be a small maintenance issue.

Status: Needs review » Needs work

The last submitted patch, 3: accessCheck-2695983-3.patch, failed testing.

socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented

Huh. It passes locally, even on the latest dev, but testbot doesn't like it.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented 
+++ b/flag.services.yml
@@ -1,4 +1,12 @@
+  access_check.flag.action:
+    class: Drupal\flag\Access\FlagAccessCheck
+    tags:
+      - { name: access_check, applies_to: flag_access }
+  access_check.unflag.action:
+    class: Drupal\flag\Access\UnFlagAccessCheck
+    tags:
+      - { name: access_check, applies_to: unflag_access }

why no have a single check with flag/unflag as value instead of that super-weird quoted TRUE. We need a value anyway, and that seems easier to me.

You can access the value with $route->getRequirement(), see \Drupal\node\Access\NodeRevisionAccessCheck::access for an example.

socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented

@berdir They are treated as separate operations, so it kinda makes sense to have a separate flag and unflag access check. It doesn't make it look less weird in the routing.yml though...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

"_entity_access: node.delete" and "_entity_access: node.update" are separate operations too, seems conceptually quite similar to me?

socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented 
+++ b/src/Access/UnFlagAccessCheck.php
@@ -0,0 +1,34 @@
+ * Contains \Drupal\flag\Access\\UnFlagAccessCheck.

Extra backslash.

martin107’s picture

martin107 CreditAttribution: martin107 commented

Going back to the original - I think I missed at step.

and looking at the uses of _node_add_access

node.add:
  path: '/node/add/{node_type}'
  defaults:
    _controller: '\Drupal\node\Controller\NodeController::add'
    _title_callback: '\Drupal\node\Controller\NodeController::addPageTitle'
  requirements:
    _node_add_access: 'node:{node_type}'
  options:
    _node_operation_route: TRUE
    parameters:
      node_type:
        with_config_overrides: TRUE

I think _flag_access should be changed

- _flag_access: 'TRUE'
+ _flag_access: 'entity:flag{flag}' or something - it is so badly documented that I am having trouble figuring it out.

[ yes having the string 'TRUE' in a yml file is a commonly used drupal madness that yml's creators tried so hard to avoid. ]

martin107’s picture

martin107 CreditAttribution: martin107 commented

@socketwench regarding #5

I can repeat the failures -

for reference here is my setup

php7.0
drupal 8.2.x
opcode cache: on
nginx/1.8.0
mysql 5.5.42

martin107’s picture

martin107 CreditAttribution: martin107 commented
FileSize
accessCheck-2695983-12.patch6.39 KB
interdiff-2695983-3-12.txt3.09 KB

LinkTypeFiedEntryTest now passes ... I suspect the other tests will also now work.

Regarding Berdir concerns about having a single check for both flag and unflag.

I would rather create 2 similar class with all the associated maintenance issues that creates for the sake of a clean linear flow through through methods without if statements.

I appreciated not every likes this style ...for the moment I am going to leave this issue as a straight conversion project.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Needs work » Needs review
socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented
Status: Needs review » Reviewed & tested by the community

I would rather create 2 similar class with all the associated maintenance issues that creates for the sake of a clean linear flow through through methods without if statements.

I much prefer this approach, even if it comes at the cost of an additional class. Thank you!

joachim’s picture

joachim CreditAttribution: joachim commented

> "_entity_access: node.delete" and "_entity_access: node.update" are separate operations too, seems conceptually quite similar to me?

Shouldn't have left it so long to say something... I was thinking I agree with Berdir's comment above.

But the patch is ready and we have one maintainer who likes it, so that's fair enough.

socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented

I'd rather treat flag/unflag as separate operations conceptually when designing APIs, since it leaves open the (unlikely) possibility of other operations.

  • martin107 authored c1008df on 8.x-4.x 
    Issue #2695983 by martin107: Changed FlagAccessController into two...
socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented
Status: Reviewed & tested by the community » Fixed
socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented

Thanks everyone! If there's any further changes, let's split those off into other issues. I want to unblock #2694755: Top level Flag namespace is too crowded

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.