Problem/Motivation

As follow-up of #3169639: D9: Individual node permissions don't work

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#10 3236465-10.patch3.97 KBjungle
#3 3236465-3.patch8.19 KBjungle
#3 interdiff-2-3.patch1.18 KBjungle
#2 3236465-2.patch7.93 KBjungle

Issue fork nodeaccess-3236465

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jungle created an issue. See original summary.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
StatusFileSize
new 3236465-2.patch7.93 KB

Upload the patch first, comments coming next.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
StatusFileSize
new interdiff-2-3.patch1.18 KB
new 3236465-3.patch8.19 KB

Oops, #2 is a wrong patch. Reuploading.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Active » Needs review 
  1. similarity index 80%
rename from services.yml

rename from services.yml
rename to nodeaccess.services.yml

    Shouldn't it be renamed to MODULE_NAME.service.yml? 

  2. +++ b/src/AccessChecks/NodeGrantAccessCheck.php
@@ -2,7 +2,7 @@
-use Drupal\Core\Routing\Access\AccessInterface;

@@ -10,7 +10,24 @@ use Drupal\node\Entity\Node;
-class NodeGrantAccessCheck implements AccessInterface {

    AccessInterface no longer defines any methods. See https://www.drupal.org/node/2266817. Removed. 

  3. +++ b/src/Form/GrantsForm.php
@@ -346,9 +377,11 @@ class GrantsForm extends FormBase {
-    \Drupal::entityTypeManager()->getAccessControlHandler('node')->acquireGrants($node);
...
+    $node_access_control_handler = $this->entityTypeManager->getAccessControlHandler('node');
+    $node_access_control_handler->acquireGrants($node);

    Refactored to pass the Drupal check analysis

timodwhit made their first commit to this issue’s fork.

timodwhit opened merge request !2

timodwhit’s picture

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

I have reviewed the commit and made additional changes. All looked very good, just simplified some of the grant access check logic by removing the service that was only used on the form, and making sure Node:: and NodeType:: were not used.

MR is open

alison’s picture

alison
she/her
commented
Status: Reviewed & tested by the community » Needs review

@timodwhit I can't look now so I'll just ask: Did you include what @jungle suggested in #4?

@jungle can you review #6/#7, or the MR itself? (rather than @timodwhit reviewing their own commits :) )

Thank you both!

alison’s picture

alison
she/her
commented
Status: Needs review » Postponed (maintainer needs more info)

Hmm I think we ended up getting this work done in #3236465.

@jungle if you agree, I'll add @timodwhit to the credits over there. If not, since 3236465 is further along at this point (🙁), I think we'll need to restart the work on this issue, after 3236465 is committed.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Title: Use dependency injection as possible » Improve issue 3145629 further
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new 3236465-10.patch3.97 KB

Hi, @alison, sorry for missed your DM on slack and being late.

  1. #3145629: Fix Coding Standards introduced bugs, see #3309679: Nodeaccess configuration suddenly failed -- committed
  2. I found that #3145629: Fix Coding Standards still has room to improve. see attached patch. So instead of reopening #3145629: Fix Coding Standards, let's rescope this one to improve it.
  3. For #4.1, and #4.2, let's do it in a new issue, the MR @timodwhit created for it has room to improve too, I will file a new issue and upload a new patch for review, and credit @timodwhit on the new issue.
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

Changes FYI

  1. +++ b/src/Form/ConfigForm.php
@@ -16,17 +17,18 @@ class ConfigForm extends ConfigFormBase {
+  public function __construct(ConfigFactoryInterface $config_factory, EntityTypeManagerInterface $entitytype_manager) {
+    parent::__construct($config_factory);

@@ -35,7 +37,8 @@ class ConfigForm extends ConfigFormBase {
+      $container->get('config.factory'),

    I think it's worthing to have $config_factory defined in \Drupal\Core\Form\ConfigFormBase injected here. Even calling $this->config() still works without doing it. 

  2. +++ b/src/Form/ConfigForm.php
@@ -16,17 +17,18 @@ class ConfigForm extends ConfigFormBase {
-   * @var Drupal\Core\Entity\EntityTypeManagerInterface
+   * @var \Drupal\Core\Entity\EntityTypeManagerInterface

+++ b/src/Form/GrantsForm.php
@@ -21,50 +21,50 @@ class GrantsForm extends FormBase {
-   * @var Drupal\Core\Database\Connection
+   * @var \Drupal\Core\Database\Connection
...
-   * @var Drupal\Core\Config\ConfigFactoryInterface
+   * @var \Drupal\Core\Config\ConfigFactoryInterface
...
-   * @var Drupal\Core\Entity\EntityTypeManagerInterface
+   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
...
-   * @var Drupal\node\NodeGrantDatabaseStorageInterface
+   * @var \Drupal\node\NodeGrantDatabaseStorageInterface
...
-   * @var Drupal\Core\Messenger\MessengerInterface
...
-   * @param Drupal\Core\Database\Connection $database
+   * @param \Drupal\Core\Database\Connection $database
...
-   * @param Drupal\Core\Config\ConfigFactoryInterface $configfactory
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $configfactory
...
-   * @param Drupal\Core\Entity\EntityTypeManagerInterface $entitytype_manager
+   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entitytype_manager
...
-   * @param Drupal\node\NodeGrantDatabaseStorageInterface $nodegrant_storage
+   * @param \Drupal\node\NodeGrantDatabaseStorageInterface $nodegrant_storage
...
-   * @param Drupal\Core\Messenger\MessengerInterface $messenger
+   * @param \Drupal\Core\Messenger\MessengerInterface $messenger

@@ -385,6 +385,7 @@ class GrantsForm extends FormBase {
+    /** @var \Drupal\node\Entity\Node $node */
     $node = $this->entityTypeManager->getStorage('node')->load($nid);

    To make PHPStorm happy

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

Issue filed for #4.1 and #4.2 Please see and review #3316343: Refactor code and get rid of NodeGrantAccessCheck

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Title: Improve issue 3145629 further » Improve issue 3145629 -- coding standards further
Version: 8.x-1.x-dev » 2.0.x-dev
Parent issue: » #3317693: [Meta] 2.0.x branch: Working toward a stable release

  • jungle committed f6ed3ab on 2.0.x 
    Issue #3236465 by jungle, timodwhit, alison: Improve issue 3145629 --...
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs review » Fixed

Committed to the new 2.0.x branch

Status: Fixed » Closed (fixed)

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