ACL Drupal 8.x Port

Thank you for your work, id.tarzanych!

I need to create a D7 release and from that a D8 branch, but I'd like #2385183: Oracle Support to go into the D7 release first.

Also, for D8, we need to rename the 'number' column because 'number' is a reserved word. The closest we can get is probably 'figure', but I'd use that only for the database table and keep the parameters unchanged.

Thank you for offering co-maintainership. Please stick around and see this through to release. You're very new to Drupal and I can't give you commit access right away, but I'm very interested to have you on board!

Re: #2385183: Oracle Support

Both names are weird in their own way, but number_acl could more easily be misinterpreted (as having a meaning that it doesn't), so I'd prefer figure.

The trouble is I don't have an Oracle installation to investigate/test this and I don't know whether it's just that column name that causes problems or if there's more...

We'll need a D7 release and a D8 branch so that you can create a patch against 8.x-1.x-dev before I can really look at it and commit it.

Please open D8-dev branch, rather than making interested contributor wait.

What exactly does "a d7 release" mean. With my understanding of the terms, a "d7" release happened back in 2011 when 1.0 was made live. Do you mean "1.1" is overdue and you want that done, first?

Could one of the maintainers create a task with "release blockers" attached, describing what the goals are and why they're preventing any progress in d8.

It's good practice to create a tag and release before branching, so that we have a well-defined base for D8.

8.x-1.x-dev is ready now.

Wow, green at the first try!

There are some things that we should do, going from D7 to D8:

1. Update the version number in README.txt.
2. Feel free to add a line for yourself under Acknowledgements in README.txt.
3. Replace the upgrade information for D7 with upgrade information from D6 and D7 to D8 in README.txt.
4. Take the opportunity to rename the 'number' column to 'figure' as per #2385183-6: Oracle Support and #4 above. Or some other column name, if you can think of a better one that is not a reserved word.

   I wonder whether we can get away with writing an acl_update_8001() function to change the database schema (or whether we really need the Migrate API). Accroding to Upgrade API, hook_upgrade_NNN() is not run during major upgrades, but Migrate API is overkill just for renaming a column...

5. I see that you've turned a number of db_query() calls into DBTNG format, which has more overhead. We need to check whether any of these are ever executed in a loop (e.g. from Views), which could result in a severe performance loss. Node Access is a notorious performance killer, and we must be very careful that we don't make things worse!

There'll probably be more.

And we'll need testing by everyone!

Seems that we'll have to use Migrate API and create mapping.

Hmm, if I understand https://www.drupal.org/node/2186315 correctly, then every contrib needs to implement the Migrate path...

The 'number' column wasn't introduced until acl_update_7000(), and direct upgrading from D6 needs to add (not just rename) the column.

Thank you for the updated patch, id.tarzanych!

Any reviewers/testers out there?

Are there any client modules for ACL yet?

Now that CI seems to work for contribs, I've enabled the non-MySQL databases as well, and apparently they're still causing issues.

Can I ask you for feedback in the future drupal.org issue?

I'm not sure I understand -- I'll be happy to help where I can, but you're ahead of me as far as D8 is concerned...

Where did you see that?

I think the errors for pgsql and sqlite are in the database layer. The db_insert()->from($subselect) should create a parenthesis around the SELECT but apparently doesn't.

Fantastic -- this is really great!

Here are a some comments from looking through the code, in no particular order:

1. CHANGELOG.txt: Please add periods at the end of the items.

2. README.txt line 65: Change to 8.x

3. acl.install:

    'indexes'         => array(
      'module_name_figure' => array(
        array('module', 64),
        array('name', 64),
        'figure',
      ),
      'module_figure' => array(
        array('module', 64),
        'figure',
      ),

I wonder whether it would make sense to take the opportunity and reverse the indexes, i.e. have 'module_figure_name' and 'module_name', because an int index is probably more efficient than a varchar index. What do you think?

4. acl_create_acl() was somewhat permissive by allowing calls with the same arguments and returning the matching $acl_id if the record already existed. This was not documented but still a potentially useful feature. You've changed drupal_write_record() to db_insert() which would cause an error in this case. Can we use db_merge(), write a test, and document the feature?

5. Let's remove acl_create_new_acl().

6. acl_add_nodes(): We should keep the db_insert()->from($subselect) for MySQL. If the others need a clumsy work-around that should not go at the expense of MySQL.

7. _acl_module_load_include() is not used anymore, remove.

8. acl_node_access_explain() and acl_node_test_node_grants(): I like functional programming. Unless there's a clear benefit to having a variable I like avoiding one-time-use variables, especially if they lead to odd naming such as foreach ($users as $account).

9. AclWebTestCase.php: Why is this class called AclWebTestCase and not AclTest?

10. AclWebTestCase: We don't really need $web_user to be a property. I made an exception to my rule to not define one-time-use variables, because I wanted to show the familiar pattern and use the familiar name. Making it a local variable implies that we won't use any other users.

11. AclWebTestCase::setUp(): entity_create() is deprecated.

12. AclWebTestCase::setUp(): Please fix 'succesfully' to have one more 's' (wrong in the D7 version).

13. d6_d7_table.php: D6 does not have a 'number' column.

14. It seems we're only migrating the {acl} table at this point. What about the other tables?

I've added migration template, source, destination plugins, covered migration process with Simpletest.

15. The migration tests don't test anything yet, do they?

16. acl.install: It makes no sense to keep the old hook_update_NNN() implementations, does it? Please remove them.

I haven't done any actual testing yet.

Re 6.: We should be able to do something like

  if (Database::getConnection()->databaseType() == 'mysql') {
    // use db_insert()->from($subselect)
  } else {
    // The PostgreSQL and SQLite drivers currently fail to
    // generate the required parentheses around the subselect and
    // cause an error in their respective database systems.

    // use ugly and inefficient fallback
  }

Re 13.: Ah, yes, you're right. I was confused because I saw acl_update_7000() adding the 'number' column. But it was also added in acl_update_6002().

15. They test number to figure property migration

Yes, but that still leaves a lot of room for things to go wrong. We're really missing devel_node_access.module in D8 at this point.

You're right with your concern about db_merge(), and it's deprecated, too!

Also, the code in Merge::execute() is not what I was counting on. I was hoping for an SQL MERGE query, but apparently that's not what we're getting.

So I agree with going with db_insert(). Those who need the old undocumented functionality can do their own query.

This looks very good, thank you!

1. +++ b/acl.admin.inc
   @@ -5,32 +5,50 @@
   -  if (!isset($label)) {
   -    $label = (isset($record['name']) ? $record['name'] : (isset($record['number']) ? $record['number'] : $acl_id));
   +
   +  if (!empty($label)) {
   

   !empty() is wrong. empty() would be much closer, but still not the same as !isset(). We need !isset() here, so that the caller can pass '' (an empty string).

   The ultimate guide to these functions (actually they're language constructs and not functions) is http://php.net/manual/en/types.comparisons.php .

2. +++ b/acl.module
   @@ -196,38 +241,42 @@ function acl_get_id_by_number($module, $number) {
   -  ))->fetchField();
   +  return Database::getConnection()
   +    ->query("SELECT COUNT(uid) FROM {acl_user} WHERE acl_id = :acl_id", [
   +      'acl_id' => $acl_id,
   +    ])->fetchField();
    }
    
    /**
     * Determine whether an ACL has a specific assigned user.
     */
    function acl_has_user($acl_id, $uid) {
   -  return db_query("SELECT COUNT(uid) FROM {acl_user} WHERE acl_id = :acl_id AND uid = :uid", array(
   -    'acl_id' => $acl_id,
   -    'uid' => $uid,
   -  ))->fetchField();
   +  return Database::getConnection()
   +    ->query("SELECT COUNT(uid) FROM {acl_user} WHERE acl_id = :acl_id AND uid = :uid", [
   +      'acl_id' => $acl_id,
   +      'uid' => $uid,
   +    ])
   +    ->fetchField();
   

   Inconsistent formatting for fetchField() (just to show that I'm paying attention :-)). Core is inconsistent, too, but putting the arrow operator is the way to go, as you've put it everywhere else.

   (This is really a tiny nit that I wouldn't have mentioned if we didn't have #1 above.)

3. +++ b/migration_templates/d6_d7_acl.yml
   @@ -0,0 +1,14 @@
   \ No newline at end of file
   

   +++ b/migration_templates/d6_d7_acl_node.yml
   @@ -0,0 +1,42 @@
   \ No newline at end of file
   

   +++ b/migration_templates/d6_d7_acl_user.yml
   @@ -0,0 +1,30 @@
   \ No newline at end of file
   

   +++ b/tests/fixtures/d6_d7_table.php
   @@ -0,0 +1,195 @@
   \ No newline at end of file
   

   Dreditor does not like having no newline at the end of text files.

I've been wrestling with enabling OPCache in my WAMPP installation, without success so far, and I'll be offline on a business trip for the rest of the week, but I promise I'll do actual testing on Saturday.

It looks like the result list just keeps getting longer when you retest, rather than just showing the last result for each test. At this point we only have the "PHP 7 & MySQL 5.5" test failing, which we ignore for now.

Argh, whenever we set to NR, it requeues the five tests that are functional (the PHP 5.4 tests aren't), and adds five results.

So, if anyone has an interest here, this is Needs Review and testing.

You probably do not want to set every single environment to issues and commit then.
https://www.drupal.org/node/87985/qa

Ideally you have just mysql5.5 and php5.5 set to issues and commit, and the rest set to just branch on commit. That way every patch doesnt spin up five testbots to over validate potential work in progress, and when that work gets comitted, it can warn of any other outstanding regressions.

I put the empty lines back into README.txt and CHANGELOG.txt and updated the latter. No other changes to #68.

Thank you for all your work and your perseverance, id.tarzanych!

We have a great baseline with your patch — I've tagged it as 8.x-1.x-alpha1.

Testing continues in #2596089: ACL for D8 — ALPHA1.

(Mixologic: Thanks for your hints in #80.)

Can I ask you for your review and feedback in the future?

Yes, sure. I can't guarantee fast follow-up though...

Also, as far as CA is concerned, I don't know its internals and I'm not sure whether the D7 version is really working, which makes things pretty difficult... I've posted #2596099: Create a D8 snapshot release to nudge fago -- hope it helps...

You asked for co-maintaining ACL for D8 -- I'd be happy to have you on board and I've added your ID to the list. Please take a look at Best practices for co-maintaining projects.

Do you know Devel Node Access? I consider it pretty much indispensable for working on a node access module, but unfortunately I haven't had time to work on the D8 version. Would you be interested to give it a try?

But I see that it is already ported, at least partly.

Yes, somewhat, but it's not working at all right now:
#2380335: Devel Node Access block is broken
#2208875: Fix fatal errors in devel_node_access following hook_menu removal