The Rules daily tests pass at 8.5 but fail at 8.6 and 8.7. This issues is a spin-off from #2989050: 'No reaction rule' message uses singular form in 8.5 but plural form in 8.6

Since 14 July six of the test failures are in:

Drupal\Tests\rules\Kernel\Engine\MetadataAssertionTest
Drupal\Tests\rules\Kernel\Engine\AutocompleteTest
Drupal\Tests\rules\Kernel\RulesEngineTest

which all have error:
Argument 1 passed to Drupal\Core\Plugin\Context\EntityContext::fromEntity() must implement interface Drupal\Core\Entity\EntityInterface, null given, called in /var/www/html/core/modules/user/src/ContextProvider/CurrentUserContext.php on line 58

This problem is cause by the core update in #2932462: Add EntityContextDefinition for the 80% use case (thanks to TR for doing this research)

CommentFileSizeAuthor
#7 2989417-7-current-user-context.patch2.18 KBtr
#5 2989417-5-current-user-context.patch1.99 KBtr
#4 2989417-4-current-user-context.patch1.41 KBtr

Comments

jonathan1055 created an issue. See original summary.

tr’s picture

tr commented

As a heads up - I did figure out the problem, and it's a core problem, as expected, introduced by #2932462: Add EntityContextDefinition for the 80% use case. I haven't had a chance to post a new core issue because it's not just one little simple fix - the whole implementation of that issue poses problems for Rules and Typed Data. I suspect it will be difficult to get any interest from core developers for fixing that, but I'll try.

The short summary is that issue deprecates this usage when dealing with Entities:
$context = new Context(new ContextDefinition('entity:user', $this->t('Current user')), $current_user);
and replaces it with this usage:
$context = EntityContext::fromEntity($current_user, $this->t('Current user'));
And the problem is that the replacement does NOT function the same way.

Specifically, new Context() allows the second parameter ($current_user, in this case) to be NULL. In fact, the DEFAULT value of that second parameter in the constructor is NULL, so the code for Context explicitly allows and expects that second parameter might be NULL and works properly when it is NULL.

EntityContext::fromEntity(), on the other hand, does NOT allow that parameter ($current_user, in this case) to be NULL, and fails miserably if it is.

This is the failure we're seeing in our Kernel tests. CurrentUserContext is being created with a NULL $current_user (Rules doesn't create the CurrentUserContext, it's a global context which, in Kernel tests without a browser session, gets sent a NULL $current_user (as you might expect ...).

So there's this one issue where the "new" way of doing things doesn't work the same way as the "old" way, which is a regression. But there's also the bigger problem that with this deprecation we will no longer be able to treat all Typed Data data types the same way (important for Rules since we manipulate these types programmatically). Instead, we will have to check the type first, and if it's an entity type we will have to create the Context in a different way than if it's a non-entity type. (The old way still works for non-entity types).

Curiously, the error we see here is VERY similar to a problem we had about six months ago - #2936553: Rules kernel tests fail with "Creating default object from empty value"

tr’s picture

tr commented
Related issues: +#2936642: Getting runtime contexts will generate an E_WARNING for anonymous users, +#2936553: Rules kernel tests fail with "Creating default object from empty value"
tr’s picture

tr commented
Status: Active » Needs review
StatusFileSize
new 2989417-4-current-user-context.patch1.41 KB

Curiously, the error we see here is VERY similar to a problem we had about six months ago - #2936553: Rules kernel tests fail with "Creating default object from empty value".

Actually, for all intents and purposes it's the SAME problem, and #2932462: Add EntityContextDefinition for the 80% use case introduced a regression that has caused this same problem to resurface.

While the issues I raised in #2 still need to be addressed in core, I'm attaching a simple patch that should allow Rules tests to avoid the core problems and run without error.

To explain what the patch does and why it's needed, I'm going to (mostly) repeat my comment from #2936553-7: Rules kernel tests fail with "Creating default object from empty value":

The user module makes certain assumptions that must be satisfied for user to work properly. Specifically, user needs the presence of AT LEAST ONE user entity in the database. That is an additional requirement that's not true of other core modules like node - node will run just fine without any nodes in the database.

But the user module ASSUMES that there will always be an anonymous user, and if it's not there we get this test failure.

This is a problem for Kernel tests, because in Kernel tests modules aren't fully installed. Rules tests load the user module and install the user Entity schema, and for all other core modules (again, like node) that's enough. But for the user module we really have to completely install it.

Fortunately this is a simple fix - in short, we just call user_install() which sets up the anonymous user and the admin user.

There are other ways to do this - we could explicitly do a User::create() to create an anonymous user, for instance, but I think we should just call user_install() and leave it to the user module to do whatever it thinks is necessary. If that 'whatever' changes in the future, we will still be protected as long as we just call user_install().

tr’s picture

tr commented
Assigned: Unassigned » tr
StatusFileSize
new 2989417-5-current-user-context.patch1.99 KB

Looks like I missed one spot. Updated patch attached.

jonathan1055’s picture

jonathan1055 commented

Thanks for all your research. Patch solves it neatly.

In the comments I think it would be useful to add @see https://www.drupal.org/project/rules/issues/2989417 because you have put a lot of good background info in this issue.

tr’s picture

tr commented
StatusFileSize
new 2989417-7-current-user-context.patch2.18 KB

Will the added comment suggested by #6:

  • TR committed 4d07549 on 8.x-3.x 
    Issue #2989417 by TR: Argument 1 passed to \EntityContext::fromEntity()...
tr’s picture

tr commented
Status: Needs review » Fixed

Committed #7.

jonathan1055’s picture

jonathan1055 commented

That's great. We now get all green for the daily testing

https://www.drupal.org/node/190124/qa

There is one test combo which shows red, but that is for an old patch, 28 July using php7.2 & MySQL 5.5, Drupal 8.6.x. We need to get rid of this now, as as first glance it appears that one set of tests is still failing.

The display and maintainance of automated testing config on D.O. has always been a bit dodgy, with the three characteristics of core version + php version + mySql version not being used as a unique key. I recall raising an issue about it before, but can't find it now. The Rules test page does not display the latest 8.6 result by default, but when you click through you see them.

tr’s picture

tr commented

Yes, the testing is all messed up. I've been trying to fix that but it doesn't run the tests I select and I can't seem to overwrite the one-time fail - If I run a test with those exact same parameters it sticks the results in a different place. Also, the 'test on commit' tests is supposed to run under D8.5.6, but if you look at it you see that it sometimes runs D8.7. I'll probably just delete all the tests and add them back one-by-one to try to get some reasonable set.

I think that other issue is #2927534: More automated test runs on drupal.org? We can continue this discussion over there.

Status: Fixed » Closed (fixed)

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