Automated Drupal 10 compatibility fixes

Adapting the tests is nice, but I think the major blocker on D10 compatibility is #3300360: Integration with the new per-content-type "Manage permissions" tab in Drupal core

As noted in my comments to #3300360: Integration with the new per-content-type "Manage permissions" tab in Drupal core, this is cosmetics only, and has nothing to do with D10 compatibility.

Would be great if D10 compatibility were at least available in the dev release, as e.g. my module default_content_access depends on it and is waiting for a release.

I am unable to make make heads or tails out of the changes suggested to the functional tests in the Rector patch in comment #6, but I've added the core_version_requirement suggested.

I've pushed this in a dev release that you should be able to install on a Drupal 10 website. I may push a tagged alpha release after some more testing.

I'd love to see a tagged alpha :)

@gisle are the patch bits you're wondering about just the removals off $this->pass('No ACL module present, skipping test');? Perhaps they aren't necessary. I am wondering if the core_version_requirement change is all that's necessary. Are tests passing after that change alone? I can probably test that change on dev branch if it would be helpful to you and that's all that is necessary to move this through.

Thanks for the help!

@gisle I have tested your updated dev branch and it seems good to me.

I am adding a patch to remove the offending $this->pass() from the tests. Since acl is a dev dependency (I've added the necessary repositories declaration to composer.json to allow it to install), the module should not be missing, and hence the tests should never need to be skipped. Removing this code is required to pass the Drupal 10 Upgrade Status report.

I think this is ready to go.

here is a more friendly version of the test fix patch. It doesn't include the change to composer.json, as I am guessing there is some reason you all had that file set up as it was (I'm happy to add that change back if you think I should).

In this patch I also retained the checks for the presence of acl module in the setup() methods on the two tests in question. Now it will throw an exception with a meaningful message so that anyone running tests and failing for this reason will know that they should install/enable the ACL module in their environment to resolve the issue.

The tests had redundant checks for the presence of theacl module, as they both checked for it in their respective setup() functions and then again in each actual test functions. The way phpunit works is that setup() gets run before each tests, so there is no need to execute the check again in the actual test functions... having this logic in setup() covers all test methods in the class.

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

@gisle, I am unable to verify tests, as the phpunit.xml file in the test runner is outdated and needs to be migrated to work with phpunit 9. I do not see this file in the code base, so I am guessing it's stored in some drupal.org module test config. Can you update the phpunit.xml so we can verify the automated tests?

Where is the patch or TR you want to see applied?

@gisle you mentioned here that you were unsure how to handle the Rector patch proposal to remove instances of $this->pass(...) in the associated test files.

I'm submitting a cleaner content_access-Drupal10-3286723-17.patch patch to address that.

If accepted, that should resolve the D10 compatibility of this module, which should allow us to cut a new release for D10 users and close this issue.

It also means you can close this duplicative issue with a similar patch: https://www.drupal.org/project/content_access/issues/3226603.

I believe my patch is preferable because the patch there is redundant, as was the original testing code. The way PHPUnit works is that setup() gets called before each test*() function on the test class. So we only need to check for the presence of ACL once, in the setup() function, as in content_access-Drupal10-3286723-17.patch.

If we merge an release, you can also close: https://www.drupal.org/project/content_access/issues/3351148

For module to pass with on D10, $defaultTheme cannot be classy, also fixed a 10.1.x deprecation :)

Sadly, it can't be tested on DrupalCI as of now, as ACL must be compatible first

marking RTBC because #19 works for me.

@Taran2L, you removed D8 support from info.yml. Does the patch actually break D8.8, or did you remove it just because D8 is no longer supported? In either case, if we want to drop D8 from info.yml, I'm wondering if we should publish a new major release (2.0.0 or 2.0.0-alpha). As for testing D10 and the ACL blocker, I am working with @salvis on getting ACL up to speed and publishing a new release. Hopefully we can finish that up in the next week or two.

@gisle, what do you think? What exactly do you need from us to publish a new D10 compatible release? I am not sure how active @fago is these days, or if he is still monitoring this queue (I have yet to discover an effective way of finding a chronological list of a user's most recent activity on drupal.org).

@Taran2L, not sure why to support D8 anymore (I'm not sure whether it still works with D8 or not, but one cannot test it on DrupalCI anymore anyways).

Then, regarding a new major version - don't see a reason for this. API (i.e. promise to the consumers) of the module has not changed, and internals do not matter. D8/9 users will stuck to the current alpha version => then will be able to update to D9 and receive a new version which supports D9/10. There is no way to upgrade from D8 to D10 avoiding interim step of upgrading to D9 first (its technically possible to do with Composer, but not possible from Drupal update hooks POV)

Then, regarding a new major version - don't see a reason for this. API (i.e. promise to the consumers) of the module has not changed, and internals do not matter

@Taran2L, if a site maintainer X is running D8, and we merge your patch, then we cut a new release (8.x-1.0-alpha5), when site maintainer X runs composer updates, won't it pull in 8.x-1.0-alpha5, which isn't compatible with their D8 site and cause an issue? Or perhaps composer will simply refuse to update the package, seeing that the new version does not support D8?

I am not sure myself.

@gisle?

If Drupal 8 is dropped from core_version_requirement, as suggested by the patch in comment #18, composer will refuse to install the update on a site still running Drupal 8.

Drupal 8 is beyond EOL, so if someone is still running that version, they should not expect that new releases will be compatible.

so @gisle, does that mean you are okay dropping D8 and the patch in #18 is sufficient here? If not, please provide me clear instructions and I will do whatever you request to get this thing wrapped up.

Yes, I'm OK with dropping D8.

All I need to know is if you're confident that the patch in comment #18 makes the project Drupal 9/10 compatible and don't break Drupal 9.

If that's the case, I'll push a new tagged release for Drupal 9/10.

hey @gisle, I am confident #18 makes the project Drupal 9/10 compatible without Drupal 9. A new release would be much appreciated :)

@xeM8VfDh, thanks for the review. @gisle, would be great to see this committed and a new tagged release.

thanks!

There is now a tagged release for Drupal 10: https://www.drupal.org/project/content_access/releases/2.0.0-alpha1

Those requiring Drupal 8 compatibility should use version 8.x-1.0-alpha4

thanks @gisle and @Taran2L!

About half of this was in a patch I submitted two years ago #3226603: BrowserTestBase::pass() is deprecated

It would have been REALLY nice if you had committed that two years ago, but at the very least you could give me some credit for my contribution ...

#3226603: BrowserTestBase::pass() is deprecated had no community engagement. It had still status "Needs review" until I just closed it based on your comment #30 above (I closed it as "Fixed" in order to give you credit – this issue will remain "Active" in order to keep getting Rector oversight).

I am sorry I didn't recognize the overlap between these two issues, but I simply don't have the cognitive surplus to oversee all the code and all the patches for the projects I try to keep afloat. You could have given me a heads-up when this issue surfaced.

PS: I am sure this project would to better off with a more skilled maintainer with more resources, but unfortunately, nobody has stepped up and offered to join the team. You all just have to endure me as long as there is nobody else around to do this work.

You’re doing great @gisle, we really appreciate the work!

@TR, I see you’ve been around for over 15 years with credit on a whopping 1338 issues. That’s supremely impressive!

For what it is worth, I ended up submitting a patch here that duplicated your work from the other issue without even knowing it. I was simply following deprecation messages and documentation to come to the same conclusions as you. I mention all this just to say that I don’t think anyone was trying to intentionally slight you on credit. This issue became a wholesale “make it all work on D10” issue, which ended up encapsulating the issue you submitted a patch for two years ago.

Sorry you didn’t get credit, your track record is impressive and we appreciate your contributions to the community.

Drupal 10 compatibility is complete.