Implement Content Access manager service

The code looks good, but a migration path should exist for the ~1000 installations already using 8.x-1.x-dev, to move data out of the content_access table into the new format. The patch ignores existing settings, which would be a severe regression. A content_access_update_8101() function implementing hook_update_N() should do it.

@id.tarzanych thanks a lot for the work to replace the procedural code by a service. There is a lot to gain by using services.

My comments on the patch.

1. +++ b/src/ContentAccessManager.php
   @@ -0,0 +1,538 @@
   +}
   \ No newline at end of file
   diff --git a/src/ContentAccessManagerInterface.php b/src/ContentAccessManagerInterface.php
   

   Nitpicking...

2. +++ b/src/ContentAccessManagerInterface.php
   @@ -0,0 +1,264 @@
   +   *   Grant id for role.
   +   */
   +  public function getRoleGid($role);
   +
   

   Don't use abbreviations in method names. 'Gid' -> 'GrantId'

3. +++ b/src/ContentAccessManagerInterface.php
   @@ -0,0 +1,264 @@
   +   *   Permission name or FALSE if permission is suitable.
   +   */
   +  public function getPermissionByOp($op, $type);
   +
   +  /**
   +   * Return content_access' settings.
   

   Don't use abbreviations in method names. 'Op' -> 'Operation'

4. +++ b/src/ContentAccessManagerInterface.php
   @@ -0,0 +1,264 @@
   +   *   Processed grant parameters.
   +   */
   +  public function proccessGrant($grant, $gid, NodeInterface $node);
   +
   

   Typo: not 'proccess' but 'process'

5. +++ b/src/ContentAccessManagerInterface.php
   @@ -0,0 +1,264 @@
   +   * @return int
   +   *   Status of match.
   +   */
   +  public function matchesOwnOp(NodeInterface $node, array $any_roles, array $own_roles);
   +
   +  /**
   

   Don't use abbreviations in method names. 'Op' -> 'Operation'

6. +++ b/src/ContentAccessManagerInterface.php
   @@ -0,0 +1,264 @@
   +   *   Array of role ids.
   +   */
   +  public function getRidsPerNodeOp($op, NodeInterface $node);
   +
   

   As above use 'getRoleIdsPerNodeOperation' instead of 'getRidsPerNodeOp'

7. +++ b/src/ContentAccessManagerInterface.php
   @@ -0,0 +1,264 @@
   +   * Returns the per node role settings. If no per node settings are available,
   +   * it will return the content type settings.
   +   *
   

   Method descriptions should fit on one line. Additional info to be separated with an empty line.

   Description is plural ("... settings.") and method name is singular. This is confusing.

8. +++ b/src/ContentAccessManagerInterface.php
   @@ -0,0 +1,264 @@
   +   * @note
   +   *   This function won't apply defaults, so if there are no other settings
   +   *   it will return an empty array.
   +   *
   

   Hmm, on a 'get' method I don't expect defaults to be added. (Without reading the code) this note causes more confusion that it is worth.

9. +++ b/src/ContentAccessManagerInterface.php
   @@ -0,0 +1,264 @@
   +   */
   +  function getPerNodeSettings(NodeInterface $node);
   +
   

   Missing 'public'

   The is not a clear distinction between getPerNodeSetting and getPerNodeSettings, but the description of with _and_ the different variables suggest differently. I suggest to use a better name for one or both.

10. +++ b/src/ContentAccessManagerInterface.php
    @@ -0,0 +1,264 @@
    +   *   Array of Content Access settings.
    +   */
    +  public function savePerNodeSettings(NodeInterface $node, array $settings);
    +
    

    I suggest to rename to 'setPerNodeSettings'. That is more consistent with get(PerNode)Settings and setSettings.

11. +++ b/src/ContentAccessManagerInterface.php
    @@ -0,0 +1,264 @@
    +   * Removes grants that doesn't change anything.
    +   *
    

    Typo: "Removes grants that don't change anything."

12. +++ b/src/ContentAccessManagerInterface.php
    @@ -0,0 +1,264 @@
    +}
    \ No newline at end of file
    diff --git a/src/Form/ContentAccessAdminSettingsForm.php b/src/Form/ContentAccessAdminSettingsForm.php
    

    Pls

13. +++ b/src/Form/ContentAccessPageForm.php
    @@ -83,23 +113,24 @@ class ContentAccessPageForm extends FormBase {
    -    $form['reset'] = array(
    +    $form['reset'] = [
           '#type' => 'submit',
    -      '#value' => t('Reset to defaults'),
    +      '#value' => $this->t('Reset to defaults'),
           '#weight' => 10,
           '#submit' => ['::pageResetSubmit'],
    

    Changes like these are not related to using a service instead of procedural code. Imho they should not be in this patch, it causes noise dring the review process.

I don't like the introduction of a custom table to store settings. Further it extends the scope of this issue too much. Introducing a service is already pretty big, but only technically. Now we are about to to start a discusion on the storage of settings (configuration). I suggest to remove it from the patch and make a follow-up. That way we can the discuss that separately and keep the 'service' moving.

Unassigning to allow others to finalize this.

This should go into the version compatible with Drupal 10.

Sorry to do this, but I'm going to close this very old issue down.

I suspect we'll get there eventually, but I think this issue was trying to do too much all in one go.

If someone wants to be inspired by it, lets pick off the logical pieces one-by-one rather than trying to do it all in one monster patch.