[meta] Make D7 rules compatible with PHP 8.0/8.1

Is Drupal core compatible with PHP 8.0 yet? Did I miss the change notice?

Please provide specifics about what needs to be changed, and preferably a patch.

The current core version is priority - after that is addressed we can work on D7.

What I was perhaps too subtly implying in #2 is that this is indicative of a communication problem with core Drupal, where many important core changes do not get communicated to contributed module maintainers. We have several mechanisms that are supposed to be followed, for example the Change Record system where we ostensibly require a CR for every change. In the case of PHP 8 compatibility, that is an extremely recent thing for core D7 to be compatible with PHP 8, but there was no CR. Likewise when core was made compatible with PHP 7.4. On the other hand, MySql 8 compatibility WAS announced with a CR, which gave us notice that we could start testing our modules and making sure they can work with MySql 8.

Contrib can't really begin to address PHP 8 until core tests green and until Drupal CI adds configurations for PHP 8 testing. Again, that happened only recently, and with no community notification it seems a little judgy to say Rules has a bug because it doesn't immediately work with PHP 8. It would be nice if core could support contrib better - a CR with a list of known PHP 8 incompatibilities and links to the issues where core fixed these would not only let contrib know that there has been a change but would prevent us from having to each and separately figure out what the incompatibilities are and how to address those incompatibilities.

I changed this to a "task" because it's ongoing work that needs to be done to address ever-changing core requirements and standards. I've changed this to a "meta" issue because there's nothing specific here - specifics should be dealt one at a time in separate issues such as #3210303: PHP 8 introduced breaking change to call_user_func_array() which I just opened.

D8 will be fixed first, then D7. It looks like the problems in D7 are mainly a bunch of things that are newly-deprecated in PHP 8, which I'm inclined to ignore by suppressing deprecation failures in drupalci.yml like Drupal does with all other deprecation issues found by the testbot. As D7 is nearing its end of life and Rules is used on a huge number of D7 sites, it's more important to me that D7 Rules continues to work and be reliable rather than it is to change code that is merely deprecated and will continue to work in PHP 8 as-is for the rest of the known lifetime of D7.

It also looks like D7 has some Drupal CI configuration problems - there are some unknown failures that I haven't looked into yet but which seem like the test environment not being set up properly. Likewise with the MR tests that you triggered. Because this works for everything but PHP 8 I suspect it's not a problem with Rules code. Regardless, these should be addressed so that the tests run green.

With #3210303: PHP 8 introduced breaking change to call_user_func_array() committed, D8 Rules is now compatible with PHP 8.0. See the test output at https://www.drupal.org/node/190124/qa
Therefore, moving this issue back to D7 to continue ...

Looking at the console output for the failed MR tests, I see:
Error: Class 'RulesLog' not found in RulesAdminMinimalProfileTestCase->setUp() (line 40 of /var/www/html/sites/all/modules/rules/rules_admin/tests/rules_admin_minimal_profile.test).
which should not be happening. RulesLog is loaded directly in rules.info using files[] = includes/rules.core.inc, and the rules module is enabled (thus the .info file read) in the line of code just before the above failure. And of course, this works on a live site and on Drupal CI just fine.

This points to something wrong with class loading in the D7 MR tests - that's something that wouldn't show up with most D7 modules but Rules uses a lot of object oriented code even in D7.

However, we won't get a clean test result in D7 until Entity module is fixed

The current maintainers of Entity API are not working on D7, they are only maintaining D8. The last D7 commit for that module was over three years ago.

There are many issues in the Entity API queue that cause problems for D7 Rules, so this is something we've been dealing with for years. This just reinforces my inclination to turn off the PHP deprecation notices so the tests will pass, but I think this might require overriding the default Drupal error handler rather than just setting a parameter in drupalci.yml.

#4 in patch form so we can test it on Drupal CI.

#8 didn't reduce the number of failed tests, but it did eliminate the deprecation failures caused by Rules. All the deprecation failures are now in the Entity API module.

Split off the MR into its own issue: #3210626: Required parameters following optional parameters

Moved this back to active to discuss what to do about the Entity API deprecations and other issues shown in the Drupal CI tests.

Added related issue for the Entity API.

The -dev version of the Entity API now has the PHP 8.0 fixes, so what is needed here for the next step is to create a patch to make the testbot use the -dev version of Entity for testing. That way we can see what is left to fix.

what is needed here ... is to create a patch to make the testbot use the -dev version of Entity

In 8.x I would do this in the drupalci.yml file, but Rules at 7.x does not have this file. I'm not sure if any 7.x modules have this file and functionality.

Is there a way to change dependencies[] = entity in rules.info, or add test_dependencies[] and somehow specify the dev version?

drupalci.yml only controls which types of tests are run, correct? It would be composer.json that controls which versions are used? Drupal CI doesn't use composer for dependencies in D7 tests, so that doesn't help with D7.

A test_dependency[] has to be committed first, it cannot be part of the patch. The problem is I don't know the correct way to specify a dev version in a test_dependency[] - it is not documented, there are no examples, I have asked (and have an issue open for many years now) and not received an answer, and I have experimented by committing .info file with different variations of the test_dependency[], without luck. (This is something that can't be tested locally - just have to guess, commit, wait for the tests to fail, guess again, commit again, wait again ...).

I don't have the energy to go through that process again. My plan is to get a new version of Entity API released so we don't have to deal with this. See #3221001: Plan for release of 7.x-1.10. There are many other modules that depend on the Entity API before they can become compatible with PHP 8.0, so I think this is a better use of time.

"drupalci.yml only controls which types of tests are run"

drupalci.yml can do a lot more than that, by using the container commands. You can apply a secondary patch to the module being tested, apply patches to 3rd-party modules or to Core if required for testing, or change the versions installed by running extra composer require commands.

"It would be composer.json that controls which versions are used? Drupal CI doesn't use composer for dependencies in D7 tests, so that doesn't help with D7."

Actually, Drupal CI does use composer.json for D7 testing. This may be a relatively new enhancement, not sure when it came in. But I have just checked this with the Scheduler module #3221881: Composer.json and drupalci.yml for non-MR branch testing. It works for MR branch testing and for non-MR patch testing.

The changes in patch 4 (in comment #8 above) have been committed in #3210626: Required parameters following optional parameters so they are no longer required in a patch here. This new patch #18 just adds a composer.json file and attempts to get the dev version of entity when running tests. Not sure if this is perfectly correct yet, but worth seeing what the d.o. testbot does.

Fix composer syntax.

That's good. The log initially has

  - Installing drupal/entity (1.9.0)

Then we get:

Updating dependencies (including require-dev)
Package operations: 0 installs, 1 update, 0 removals
  - Removing drupal/entity (1.9.0)
  - Installing drupal/entity (dev-1.x 67bc94c): Cloning 67bc94c89e

In reply to #15

"so what is needed here for the next step is to create a patch to make the testbot use the -dev version of Entity for testing. That way we can see what is left to fix."

Done. Hope this helps.
I don't have time to work on the actual Rules fixes, so over to someone else for the rest of it. :-)

Thanks for working on this! This should help a lot. It shows that there is still a problem with the Entity API module. Curious, because that error does NOT show up in the Entity API automated tests - see https://www.drupal.org/pift-ci-job/2069570

I don't know why the Rules tests would be bringing in entity.features.inc when we don't have Features tests. It might be because we declare a features controller key in hook_entity_info(), but I wouldn't think that would cause the controller class to load at that point. (If it does load, then that forces the parent class to load, which is in entity.features.inc, which is where the PHP 8 problem is...)

So the first step is to write a test for Entity API to demonstrate the problem then get another fix into the Entity API to eliminate those errors.
Then the second step is to work on the Rules-specific errors that are left.

Just an experiment to determine exactly how entity.features.inc gets pulled into our tests.

Yes, that stopped entity.features.inc from loading, without breaking Rules tests. Let's try a more minimal change.

And another verison of #23 for completeness.

*** Edit *** interesting

Entity API issues:

• #3222088: Some test cases are not being executed fixed the Entity API tests so that they run all the tests, including the Rules integration tests.
• #3222097: EntityAPIRulesIntegrationTestCase fails with PHP 8.0 shows that Entity API Rules integration test needs Rules 7.x-2.x-dev, and when that is used we can see the entity.features.inc failure with PHP 8.0.
• #3222090: Remove required parameter following optional parameter in entity.features.inc (PHP 8.0) fixes the entity.features.inc problem in the Entity API.

The remaining problems with Rules in PHP 8.0 are the 5 failures shown in #25.

retriggering patch 25 on php 8.0

(due to related contrib fixes)

#23, #24, and #25 were just test patches which commented out some Features code to try to narrow down the problem. The problem with that Features code was fixed in the Entity API in #3222090: Remove required parameter following optional parameter in entity.features.inc (PHP 8.0). So now, the patch to look at is #19.

#19 adds a composer.json that instructs the testbot to use the -dev version of Entity API when running the Rules tests.

I retriggered the testbot on #19, and as you can see we are still left with 5 test failures that seem to be entirely from Rules and not because of the Entity API or any other code. These 5 test failures need to be looked at and fixed so that D7 Rules is fully compatible with PHP 8. This is what I said in #27.

Patches to correct these remaining 5 test failures should also include the composer.json from #19 so that the -dev version of Entity API is used in the test.

With the new release of Entity API 7.x-1.10, Rules tests are now passing with PHP 8.0

However, there are still a number of fails with PHP 8.1. These fails should be handled in child issues.

agreed, great work on the PHP 8.0 fixes, would be a good strategy to cut a release now and work on PHP 8.1 in the following release

Umm, it's just the one child issue that needs to be finished for PHP 8.1, and the tests in that issue show that Rules will work with PHP 8.1 when that patch is committed. Perhaps comment on that "Needs review" issue since that's the only blocker?

I feel like all I get is complaining that the new D7 release is not done yet, but very little help in actually trying to get it done. I had to take the extreme step to push to become co-maintainer of Entity in order to make that module (which Rules depends on) compatible with PHP 8 - no one was working on that module so I had to do it myself - and now I have to be careful that the changes needed to make Entity compatible could break 400,000 sites. Working on the new release of Entity, which was a pre-requisite for a new release of Rules, also took months. Again, with very little help. This is supposed to be a community project, but in reality it seems I have to do it all by myself. But if I make a commit without waiting long enough for community input, I get criticized, and if I wait for community input, even for a few days like this child issue, I get criticized for taking to long.

It has been just 4 days since the PHP 8 compatible version of Entity was released (by me). I have already made 5 commits to Rules, each with a separate issue, a patch, and no community feedback, and I have one more pending issue again with a patch and no community feedback that needs to be finished before the next release of Rules for D7. And even then, I'd like to let it sit a few days on the off chance that at least one of the ~180,000 users of this module will perhaps will test the latest -dev before I make a new release. But that probably won't happen, and then after the release I will get complaints that it broke their site. So no, I'm in no hurry. It will be released when I feel that I've done all I can to make sure this new release doesn't cause problems.

@TR As someone who has only been watching, from the sidelines, this and several other issues where your efforts have been huge all I can say is you are a hero. I appreciate you saying that being a hero is lonely and not your intention, rather you were maybe hoping that leading by example would help motivate others in the community. Thank you for all of it including your recent comment.

Thanks TR.

As developers our work is lonely and thankless, but RULES is one of the crowning achievements of D7. So you have my thanks!

It just puts into sad contrast where we are at with D8/9. Everytime I go to migrate a site to D7>D8/9 I test some Rules implementations, finding many things are no longer even possible (loops for example) I shake my head and put it on the backburner again.

Rules 7.x-2.13 is now released.

Good work. Thank you.