Automated Drupal 10 compatibility fixes

@eelkeblok do you think this is ready to go? Do you have merge/release privileges or is this waiting on maintainers?

The merge request needs a final review, I'd say. It is similar to patch #5 but not identical. If that patch is reviewed, I guess it can be set to RTBC. I don't have maintainer status for this module, so somebody else will have to do the merge. If that somebody can do the review prior to that, that would be swell.

hey @eelkeblok, I want to see if I can test what you need me to test.

When I click "changes" in 3285916-automated-drupal-10 Compare changes, plain diff MR !5 mergeable, I am taken to drupal's internal git instance, with the proposed changes on the screen (here). If I click "code" and select "plain diff", it downloads a file called "5.diff". The naming is throwing me off due to your comment that the "merge request" is slightly different than #5. Is this the right patch I should apply/test?

Furthermore, it looks like most changes are to test files. I presume the tests are passing, and that I'm just testing if the applied patch works on my actual website given the changes to acl.info.yaml, is this correct?

My website is drupal 9.5.4 (latest D9), will testing there be sufficient? I cannot easily upgrade it to 10 and test under those conditions because of this module and two others blocking my upgrade path.

Let me know how you'd like me to proceed. Thanks :)

hey @eelkeblok. I tested 5.diff and it worked, except for one other change that was needed for the module to pass on the Drupal 10 Upgrade status → report:

diff --git a/acl.module b/acl.module
index c669718..281b388 100644
--- a/acl.module
+++ b/acl.module
@@ -108,7 +108,7 @@ function acl_remove_all_users($acl_id) {
  * $form['acl_id'] before calling acl_save_form().
  */
 function acl_edit_form(FormStateInterface $form_state, $acl_id, $label = NULL, $new_acl = FALSE) {
-  module_load_include('inc', 'acl', 'acl.admin');
+  \Drupal::moduleHandler()->loadInclude('acl', 'inc', 'acl.admin');
   $build_info = $form_state->getBuildInfo();
   $build_info['files'][] = [
     'module' => 'acl',

I'm attaching a rebuilt patch that includes this change xeM8VfDh.patch and making this RTBC

re-uploading patch file with proper naming conventions documented at: https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... →

Sorry, noticed your comments only just now.

The change for module_load_include is not strictly necessary, I mentioned it in #18. I'm not sure if this is what you mean when you say it does not pass the upgrade status report. FWIW, I've been running a site with the patch from the MR applied, on Drupal 10, for a few weeks now.

Apart from that, yes, the way you got to a patch to apply to your local sounds about right. There are ways to test the Git branch for the MR directly, but not sure if they are any easier when you have a specific site you want to test with. The changes with the patch from #5, I outlined in #17. The naming is a bit confusing, but that is simply because this is the 5th merge request for this project, and we happen to compare it to the patch from the fifth comment on this issue. Completely different things, just happen to be both 5.

I did not pay a lot of attention to the tests and just accepted the automatisch changes from the update bot. It looks like the project isn't configured to automatically run its tests (boo! 😉), so I queued some tests for your patch. I'm not sure what the reason is for the tests not running automatically, maybe they are broken, so tests failing on the patch doesn't necessarily mean your patch is bad.

Lastly, no, testing an issue that is meant to raise the compatibility to a new major Drupal version only on the previous major Drupal version will not suffice :) Maybe someone else can confirm the patch works on Drupal 10 too.

In order to submit an MR for this issue, you'll need to request push access to the issue fork for this issue. That sounds complicated, but there is a button to do so right below the issue summary. You then clone the issue fork for this issue, you are not meant to create your own fork for the project. You are welcome to push changes to the existing merge request, or of you don't feel confident, you can push a new branch with your own solution and open another merge request. I think it is best to push to the existing branch and thus update the existing MR, though. Things might become complicated, otherwise. Lot's of back-and-forth is not a problem, since when the merge request is actually merged, it will (or at least should) be squashed down to a single commit, just like when patches get applied.

In order to submit an MR for this issue, you'll need to request push access to the issue fork for this issue

I don't think the maintainers are around, so I don't think requesting push access will go anywhere.

You mentioned that the module_load_include change isn't strictly necessary, but it should be included, as many people are relying on the Drupal 10 Upgrade module to find their upgrade path, and that's an officially supported mechanism, and your patch fails that. The patch should probably also include a change to README to reflect the updated Drupal/php requirements. I can do that if you want.

Since you are running 10, would you be available to test a new patch I provide, with the module_load_include change as well as a new README change? Then you could update the merge request? The README change obviously won't fix anyhing, but there is already an outdated merge request for a change to README to specify the inclusion of Drupal 9 support. I think we should have one patch that handles it all, as it is related. Please let me know if you can test on drupal 10 and I will generate the patch, you can test, then update the merge request, then we are pretty much done until a maintainer can step in. If @salvis doesn't chime in shortly, I can try to appeal to the core team to step in.

https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →

thanks @Anybody. I've messages @salvis here a couple times with no response, and his user activity doesn't show any real recent activity (though there are a few contributions by "Anonymous (unverified)", which is odd, perhaps concerning. I've also emailed @salvis's company through the contact portal on his company website linked in his profile. Haven't heard back on that either, but that just went out yesterday.

I suspect will will likely need to have a new co-maintainer added. @eelkeblok, would you be available for that if we can push it through? I've added acl to the sheet here:

updating patch to not remove composer.json. Based on reviewing other D10 compatible modules, they do in fact have this file, they simply don't have a require property for drupal/core, so I've removed this property in acl-Drupal10-3285916-32.patch

The last submitted patch, 32: acl-Drupal10-3285916-32.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

attempt at rebuilding patch as acl-Drupal10-3285916-34.patch

The last submitted patch, 34: acl-Drupal10-3285916-34.patch, failed testing. View results →

I don't know how to satisfy the failing code styling tests. Seeing as the only failures are related to code styling, and the patch works in my environment, I am changing this back to RTBC.

I've just tested upgrading my site to drupal 10 with this module installed running the latest patch (using these instructions) and everything seems to be working fine

rebuilding patch in an attempt to satisfy failing code styling issues in #33 results →

The last submitted patch, 38: acl-Drupal10-3285916-38.patch, failed testing. View results →

Something is wrong with the patch testing configuration.

The latest patch acl-Drupal10-3285916-38.patch → builds successfully, and it resolves the code sniff errors, and there are no other errors present, but drupal.org is flagging it as "failed". The log does indicate some "No tests defined" errors, but I am not sure this is a problem and, either way, is beyond the scope of this issue.

attempting to fix PHP8.1/MySQL8/D9.5 and PHP8.1/MySQL5.7/D9.5 errors

The last submitted patch, 41: acl-Drupal10-3285916-41.patch, failed testing. View results →

updating patch to not remove composer.json

I've thought the same, but IIRC it was needed for the tests to run.

You mentioned that the module_load_include change isn't strictly necessary, but it should be included, as many people are relying on the Drupal 10 Upgrade module to find their upgrade path, it's an officially supported mechanism, and your patch fails that.

Not sure what you mean by the "Drupal 10 Upgrade module". Do you mean https://drupal.org/project/upgrade_status? It explictly marks this change as only needed when preparing for Drupal 11. I haven't done a thorough analysis of the other required changes and whether adopting *this* particular change has any consequences wrt the minimal core version ACL module can support. That could be a reason to either wait or proceed with the change. Is module_load_include() deprecated? Yes. Does it work in Drupal 10? Also yes. To quote the deprecation message from module_load_include() in the D10 codebase (emphasis mine):

@deprecated in drupal:9.4.0 and is removed from drupal:11.0.0. Use \Drupal::moduleHandler()->loadInclude($module, $type, $name = NULL).
* Note that including code from uninstalled extensions is no longer supported.

WRT push access, you don't need to be a maintainer to get push access to an issue fork. If you don't have it yet, you will find a button tto push in the issue fork box near the topi fo the issue. I already pushed it, so mine says "You have push access":

WRT co-mainainership, I agree this module looks somewhat abandoned. I am hessitant to accept co-maintainership at this time, since I don't really get to maintaining the modules I already maintain as it is.

Given that you have a big hand in the implementation, I don't think it is entirely appropriate for you to set RTBC :)

I set RTBC because I reviewed YOUR patch. I did submit cleaned up versions, but no substantial changes were made, just basic cleanup.

the topic of push access is moot at this point. The module needs new maintainers because the current maintainers are inactive.

Thank you for all your work here, xeM8VfDh!

There's nothing in this patch that would keep it from running in D8, or is there? Except for acl.info.yml.

I realize that D8 is unsupported, but I don't think it's contrib's job to willfully exclude people from updating who choose to run an old version for whatever reason. We still have sites running D6 and even D5...

Hey Salvis!! I am so happy to see you back :)

I believe you are correct re D8. Would you like me to resubmit a duplicate of the latest patch, but with D8 allowed in the info file?

I'm not sure tests would be executable on D8, given changes to the tests classes, but I am not entirely sure about that.

Here's a patch with D8 re-instated and the duplicative test changes removed (see this 📌 Replace all the occurrence of assertEqual() method, that is deprecated. Fixed ). This also removed all of the work I did in those files to resolve the issues with the code sniffer. I can re-implement those if you want, or perhaps we create a separate issue for "resolving code sniff warnings/errors", and I can tackle that.

The last submitted patch, 49: acl-Drupal10-3285916-49.patch, failed testing. View results →

The other patch is just in src/Tests/AclTest.php.

Apply the other patch, set a tag, reset the branch, then apply this patch, and diff to the tag — that way you easily get a patch without the changes in the other one.

Yes, apparently the testbot doesn't support testing against D8 anymore.

But I would take the chance to commit this (including your remaining changes in src/Tests/AclTest.php, please) in 8.x-1.x-dev.

Then we'll see about D10 — it will need a re-roll anyway.

>> Hey Salvis!! I am so happy to see you back :)

Thank you, xeM8VfDh :)

I won't stay long, but I'll try to clean this up — thank you for laying it out nicely and for your nudging...

hey @salvis, I think/hope this covers what you were asking for in #51

The last submitted patch, 54: acl-Drupal10-3285916-54.patch, failed testing. View results →

Great, thank you, xeM8VfDh!

Passed for D9.

I expected it to also pass on D8. The testbot doesn't support running patches against D8 anymore, but I was able to re-run the old 8.x-1.x-dev test with PHP 7.2 & MySQL 5.5, Drupal 8.9.x → test, and it fails with

PHP Fatal error: Access level to Drupal\acl\Tests\Migrate\d6\MigrateAclList68Test::$modules must be public
(as in class Drupal\Tests\migrate_drupal\Kernel\d6\MigrateDrupal6TestBase)
in /var/www/html/modules/contrib/acl/src/Tests/Migrate/d6/MigrateAclList68Test.php on line 13

Let's see what happens if we change this back to public...

The last submitted patch, 57: acl-Drupal10-3285916-56.patch, failed testing. View results →

Ok, now it's fine for D8 and D9.

thanks @salvis! You're on a tear, and I love it :)

Let me know if you need anything more from me here or in any other active issues to help you in the quest for a new release. We are in different time zones, but I can commit time to this over the coming weeks if you need further assistance.

I set RTBC because I reviewed YOUR patch. I did submit cleaned up versions, but no substantial changes were made, just basic cleanup.

Apologies, I thought your changes were more substantial.

WRT D8, I thought there were changes in there to fix deprecations from D9 (which would automatically mean it wouldn't run on any D8 version), but I might be mixing up my D10 ports at this point :)

Thanks for reviewing all of that @eelkeblok 👍

I believe @salvis managed to get the test runner to run the test suite against D8 on the dev branch (somehow that’s still allowed, despite no availability to run against deprecated versions on issue queues), and I believe he suggested in #61 that the patch was passing there.

I think we are getting close 🤞

Let me know if you need anything more from me here or in any other active issues to help you in the quest for a new release.

It would be great to get some feedback regarding the remaining 2 NR patches, whether they should be included or not. Let's wait a week and see...

@salvis can you link me to the issues/patches you are referring to?

@salvis can you link me to the issues/patches you are referring to?

EDIT: I see you mentioned 2 "Needs Review" issues, but I see three:

1. Edit form should use user display name not account name 🐛 [user_display_name module integration] Edit form should use user display name not account name Needs work
2. Drupal 7 to 9 acl_user migration error on incremental or update 🐛 Drupal 7 to 9 acl_user migration error on incremental or update Needs review
3. Views integration ✨ Views integration Needs review

I see you've commented recently on the first two, but the third hasn't had recent activity in 3 years. I am also not sure if you agree with or approve of that specific feature. The third one also mentions (tested) in their issue description, but the patch file does not in fact include any new test code. I think it would be ambitious for us to write tests for this code we are unfamiliar with from years ago. Ive pinged the issue to see if @Elin Yordanov is still around. Unless we get a quick reply with an eager engagement to contribute test coverage, I don't think this issue should hold up a new release as Drupal 10 coverage is more pressing.

I will look at the other two tomorrow

@salvis will you make new release with latest commit?

@salvis, how are we doing?

This is an automated patch generated by Drupal Rector. Please see the issue summary for more details.

It is important that any automated tests available are run with this patch and that you manually test this patch.

Drupal 10 Compatibility

According to the Upgrade Status module → , even with this patch, this module is not yet compatible with Drupal 10.

Currently Drupal Rector, version 0.15.1, cannot fix all Drupal 10 compatibility problems.

Therefore this patch does not update the info.yml file for Drupal 10 compatibility.

Leaving this issue open, even after committing the current patch, will allow the Project Update Bot → to post additional Drupal 10 compatibility fixes as they become available in Drupal Rector.

Debug info

This patch was created using these packages:

1. mglaman/phpstan-drupal: 1.1.35
2. palantirnet/drupal-rector: 0.15.1

@salvis

The patch provided by the bot in comment 70 is already pushed to the branch 8.x-1.x & it is compatible while verifying from Upgrade Status.

@salvis we need new release to upgrade to D10.

I know. We'll get there, but you need to be patient.

If you want to help, please test the 8.x-1.x-dev versions of ACL and Forum Access for D8.9 and D95. The faster we get these done (including releases) the quicker we'll get to create D10 branches.

This is an automated patch generated by Drupal Rector. Please see the issue summary for more details.

It is important that any automated tests available are run with this patch and that you manually test this patch.

Drupal 10 Compatibility

According to the Upgrade Status module → this patch makes this module compatible with Drupal 10! 🎉
Therefore this patch updates the info.yml file for Drupal 10 compatibility.

Leaving this issue open, even after committing the current patch, will allow the Project Update Bot → to post additional Drupal 10 compatibility fixes as they become available in Drupal Rector.

Debug info

This patch was created using these packages:

1. mglaman/phpstan-drupal: 1.1.37
2. palantirnet/drupal-rector: 0.15.1

There are compatibility issues with later PHP versions that are supported by D10. Fixing these will break compatibility with older PHP versions that are still supported by D89, and I don't want to do that unnecessarily.

Therefore this has to wait.

@salvis, this is supremely disappointing.

the module works on D10. is threre really no further discussion to be had on this? i read comments from you on other issues explaining you'd create a new branch for D10, so why can't we just maintain two branches?

@salvis, I agree with @xeM8VfDh this is disappointing.

the module works with the patches on D10. But beta2 breaks the patches, and thus does not work on D10.

D9 end of life is fast approaching.

What exactly do you mean by “D89”? Drupal 8 is already EOL for quite some time now and D9’s EOL is fast approaching, as has been pointed out. Maybe create a new major release if you’re not ready to drop D8 support in 1.x (please support at least D9.5 in the new major, though, for ease of transition; see E.g. https://mglaman.dev/blog/drupal-module-semantic-versioning-drupal-core-s...).

Thanks @salvis for your work in maintaining the module. I fully understand that you want to avoid causing errors in existing sites with known B/C breaks. A new major version of the module seems to be needed to serve D9 / D10 sites moving forward.

For users that already want to move to D10: it is possible to point Composer to the issue fork and it will download the D10 compatible version. I have updated the issue fork with the latest code from 8.x-1.x and the final changes needed for D10 compliance.

Add the following to composer.json:

{
    "repositories": [
        {
            "type": "composer",
            "url": "https://packages.drupal.org/8",
            "exclude": ["drupal/acl"]
        },
        {
            "type": "vcs",
            "url": "https://git.drupalcode.org/issue/acl-3285916.git"
        }
    ],
    "require": {
        "drupal/acl": "dev-3285916-automated-drupal-10"
    }
}

For more information see Core version compatibility fixes for modules with unmerged changes →

Thank you, @pfrenssen, that's very helpful!

almost all of the changes I was patching into beta1 to run ACL on Drupal 10 have been incorporated into beta3, which is great!

I am now successfully running ACL on D10 with the following acl-Drupal10-3285916-41.REROLL.patch patch

I have tested a client module's functions with "pfrenssen" fork and it works as expected in D10.
The client model can be added as part of the ACL ecosystem on the ACL project front page. :)

"drupal/commerce_license_access_control": "dev-3345299-updates-for-d10"
Commerce License Access Control →

DDEV 1.22.1
PHP Version 8.2.8 (8.1)
Drupal Version 10.1.2
Commerce Core 8.x-2.36
Commerce License 3.0.0
ACL": (fork) "dev-3285916-automated-drupal-10"

Is any release date planned for that fix?

Here's a ^10 version.

Now we need to look into PHP compatibility...

Thank you for your feedback, @Stizzi!

I thought there was a deprecation issue with PHP 8.2, or maybe 8.3?

I've run it with no issues on 8.1 and 8.2

I've tested the unpublished version 2.x-dev with version 2.0.0 of Content Access with Drupal 10 core, and I am happy to report that everything works as expected.

A published and tagged release of this version of ACL would be much appreciated.

@gisle, where did you find the

unpublished version 2.x-dev

? Or are you referring to latest 8,x branch with patches from this issue applied?

In any case, I agree, @salvis, a published release would be great. if it has to be alpha/beta/rc, that's fine.

No, I am referring to the 2.x branch. It is not visible on the project page, but it can be downloaded with:

composer require 'drupal/acl:2.x-dev@dev'

Thank you for the positive feedback @gisle!

2.x is now the default branch. 2.x-dev is visible on https://www.drupal.org/project/acl/releases → , but d.o doesn't let me publish it on the front page.

I tried to run a test with PHP 8.2 and it failed miserably: https://www.drupal.org/pift-ci-job/2759320 →

@salvis is the PHP 8.2 compatibility also a release blocker for the 2.x branch? If so we can handle it as part of this issue. Otherwise we could split off to a separate issue.

It looks like the test failure is due to a network connectivity problem. The log mentions that an RPC call failed in Curl. Let's run that test again.

Assigning to me, the MR needs to be rerolled against 2.x.

Rebased the MR against 2.x and posted this in a new MR: https://git.drupalcode.org/project/acl/-/merge_requests/10

The old MR (https://git.drupalcode.org/project/acl/-/merge_requests/5) is now obsolete and can be closed.

I'm happy to report that the tests on PHP 8.2 are green. Thanks to the 2.x branch already declaring Drupal 10 support, the MR is now also down to 2 very simple to review lines: https://git.drupalcode.org/project/acl/-/merge_requests/10/diffs

This looks ready to merge. I am going to declare this RTBC. My involvement in this issue has been to manage the MRs. I performed merges and rebases, but the code changes are authored by @eelkeblok. This has been tested by several community members including me.

Re: comment #92:

salvis, have you tried to click "Administer releases" on the bottom of the project page, then check the box "Supported" for the 2.0.x branch, and then click "Save"?

2.x is now the default branch. 2.x-dev is visible on https://www.drupal.org/project/acl/releases → , but d.o doesn't let me publish it on the front page.

@salvis, you need to add new release here:
https://www.drupal.org/node/87985/edit/releases →

Then select new dev branch and click Next.

I see 2.x on the releases page, as referenced by @salvis: https://www.drupal.org/project/acl/releases →

but yes, I agree that following the steps provided in #99 would be nice. beyond simply showing the 2.x dev branch, it would also be nice to have an alpha/beta/rc release for 2.x to avoid pulling from dev-master or pinning to a specific commit.

Thank you for the help, but...

I have
[X] Show in project’s download table when branch is supported
checked on the -dev snapshot, but the Supported checkbox is disabled, most likely because we don't have any release yet:

Thank you for the new MR, @pfrenssen, this is what I needed, and All for getting here.

I'll create a BETA1 release.

We have a failing test that should pass:
https://www.drupal.org/pift-ci-job/2770495 →

Any ideas why this is failing?

hey @salvis. I still do not see the 2.x release on the ACL module home page. Likewise, I see no beta release.

As for the errors in the test, it may be an issue with the test environment. a cursory glance makes it seem to me like the version of Drupal and the version of Symfony are not compatible. I'd be happy to look further into this if you can assist me in setting up a test environment to duplicate the errors. Drupal being what it is, I cannot simply clone the 2.x branch locally and run tests. Or perhaps there some official documentation somewhere you can point me to... it's been a while since I've done this process locally.

@salvis thanks for merging! The test that is failing now on the 2.x-dev main branch is an "updated deps" test. This is probably not caused by the merging in of this issue, but rather due to new versions being released of our dependencies in recent days.

It is important to look into since it means that users who update their dependencies to the very last versions might run into trouble with the module, but it would be best to handle it in a dedicated issue.

Thanks @pfrenssen for the clarification! I opened a new issue: https://www.drupal.org/project/acl/issues/3389630 🐛 [2.x] Failing updated deps test Fixed

Thank you for 🐛 [2.x] Failing updated deps test Fixed , @xeM8VfDh!

I'm setting this one back to Fixed.

if you can assist me in setting up a test environment to duplicate the errors.

I'm running the commercial MAMP Pro under Windows, which is pretty powerful in the sense that it allows setting up any number of combinations of Drupal/PHP/DB/etc versions side by side, along with PhpStorm, but I've always had trouble trying to reproduce the results of the testbot, or even to get tests to pass locally that passed on the testbot and vice versa. Oh, and Core|Testing is very slow, even though I have a pretty fast machine. I've never been able to figure out why.

Drupal being what it is, I cannot simply clone the 2.x branch locally and run tests.

Well, for manual testing, here's what I do:

1. and the desired Drupal version
2. run
3. open , creating an sqlite database
4. and the corresponding ACL branch into the modules directory
5. install ACL as usual

I can't think of any Drupal-specific hurdles, you have a working local LAMP environment with the correct PHP version → . Why can you not "simply clone the 2.x branch locally and run tests"?

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