Automated Drupal 10 compatibility fixes

@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 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 not be skipped.

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 has that file set up as it is (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 has redundant checks for the presence of the `acl` module, as they both checked for it in setup() and then again in each actual test. 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 📌 Automated Drupal 10 compatibility fixes Active 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 📌 BrowserTestBase::pass() is deprecated Fixed .

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 📌 Maintainership status of this module? Closed: outdated

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 #19 makes the project Drupal 9 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 📌 BrowserTestBase::pass() is deprecated Fixed

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 ...

📌 BrowserTestBase::pass() is deprecated Fixed had no community engagement. It had still status "Needs review" until I just closed it based on your comment #30 above (I closind 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 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.