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.