Problem/Motivation

Node::access() was introduced back in #1938314: Convert book_export to a new-style Controller but the purpose for its existence is no longer valid. It just overrides the parent implementation and then calls the same code anyway.

Proposed resolution

Remove Node::access().

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#19 2752267-19.patch842 bytesdavic
#15 2752267-15.patch483 byteshandrus
#10 2752267-10.patch883 bytesanicky
#8 2752267-8.patch1.68 KBanicky
#7 2752267-7.patch814 bytesanicky
#2 drupal-remove_node_access_method-2752267-2-D8.patch791 bytessagar ramgade

Comments

tstoeckler created an issue. See original summary.

sagar ramgade’s picture

sagar ramgade commented
Status: Active » Needs review
StatusFileSize
new drupal-remove_node_access_method-2752267-2-D8.patch791 bytes

Yes it has the duplicate code of parent's class method, patch attached removes it.

Status: Needs review » Needs work

The last submitted patch, 2: drupal-remove_node_access_method-2752267-2-D8.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Ahh so Node::access() provides a default value but AccessibleInterface::access() does not. So we still have to leave the function around as it is, but the function body can just be return parent::access(...);.

I guess we should also deprecate the method then so we can remove it D9. Not sure whether we should do that in this issue.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue tags: -Quick fix
anicky’s picture

anicky commented
Assigned: Unassigned » anicky
anicky’s picture

anicky commented
Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
StatusFileSize
new 2752267-7.patch814 bytes

As @tstoeckler said, I replaced the method with return parent:access(...), because of the default value.

anicky’s picture

anicky commented
StatusFileSize
new 2752267-8.patch1.68 KB

It would also may be possible to remove Node:access() if the default value is added in the parent class.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Needs work

@Anicky: I think #8 would be a larger change. I think for Drupal 9 we should rather always call the operation explicitly, and not rely on a default value. That's why I think #7 is preferable. But we should to change the indentation of the parent::... call to be inline with the coding standards. Thus, marking "needs work".

anicky’s picture

anicky commented
Status: Needs work » Needs review
StatusFileSize
new 2752267-10.patch883 bytes

@tstoeckler : Indentation fixed, sorry for that.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

No need to apologize ;-), looks perfect now. Thanks a lot!

anicky’s picture

anicky commented
Issue tags: +DevDaysMilan
anicky’s picture

anicky commented
Assigned: anicky » Unassigned
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

The patch looks good but I think we should have comment saying that the override exists to set the default value on $operation otherwise it looks like #8 is the way to go. I think we should also file a 9.x issue to remove the override.

handrus’s picture

handrus commented
StatusFileSize
new 2752267-15.patch483 bytes

Just added the comment as per #14 suggestion

handrus’s picture

handrus commented
Status: Needs work » Needs review
Rafael Natal’s picture

Rafael Natal commented
Status: Needs review » Reviewed & tested by the community

Just reviewed, its just a comment. This was a effort made during drupalcamp Campinas code sprint.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Reviewed & tested by the community » Needs work

Unfortunately the latest patch is not quite correct.

A) It does not include the previous patch from #10.

B) The code comment does not comply with the Drupal coding standards. It should be formatted like

/**
 * {@inheritdoc}
 *
 * < Put comment here >
 */

C) This is super minor, but I think it would be helpful to put the word 'view' in quote in the comment. That makes the sentence clearer to read, in my opinion.

Thanks!

davic’s picture

davic commented
StatusFileSize
new 2752267-19.patch842 bytes

This patch contains the combination of #10 and #15, together with the changes needed for the code to comply with Drupal coding standards.

davic’s picture

davic commented
Status: Needs work » Needs review
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Yay, thanks a lot. Looks great!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 241a653 and pushed to 8.2.x. Thanks!

diff --git a/core/modules/node/src/Entity/Node.php b/core/modules/node/src/Entity/Node.php
index 7222613..bdb8050 100644
--- a/core/modules/node/src/Entity/Node.php
+++ b/core/modules/node/src/Entity/Node.php
@@ -176,10 +176,9 @@ public function getType() {
 
   /**
    * {@inheritdoc}
-   *
-   * This override exists to set the operation to the default value "view".
    */
   public function access($operation = 'view', AccountInterface $account = NULL, $return_as_object = FALSE) {
+    // This override exists to set the operation to the default value "view".
     return parent::access($operation, $account, $return_as_object);
   }

Unfortunately adding additional information to an @inheritdoc is not permitted so I just moved the comment.

  • alexpott committed 241a653 on 8.2.x 
    Issue #2752267 by Anicky, handrus, davic, Sagar Ramgade, tstoeckler:...

Status: Fixed » Closed (fixed)

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