The Rules daily tests all still pass at 8.5 but fail at 8.6 and 8.7. We've had 2 test fails daily from 21 March 2018 up to 13 July. Then we've had 8 test fails daily since then. The fails are:

Drupal\Tests\rules\Functional\UiPageTest

has error:

Drupal\Tests\rules\Functional\UiPageTest::testReactionRulePage
Behat\Mink\Exception\ResponseTextException: The text "There is no Reaction Rule yet." was not found anywhere in the text of the current page

and

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

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

I suspect that the 'no text' error was originally the cause of the 2 fails. and then the EntityInterface problem is the next 6 fails.

[edit] This issue now covers the text string problem only.
See #2989417: Argument 1 passed to \EntityContext::fromEntity() must implement EntityInterface for the EntityInterface errors.
CommentFileSizeAuthor
#20 2989050-remove-todo.patch971 bytestr
#12 2989050-12.patch717 bytestr
#8 2989050-8.patch904 bytestr
#2 2989050-2.patch558 bytestr

Comments

jonathan1055 created an issue. See original summary.

tr’s picture

tr commented
StatusFileSize
new 2989050-2.patch558 bytes

Thanks for opening the issue.

The first two are due to a core change made in #2942588: EntityListBuilder::render() should use the entity type plural label in the empty text

As is typical with core development these days, no change record was created for this modification, therefore leaving contributed module maintainers to experience the test failures and figure out for themselves what was changed in core.

Let's see if this fixes it.

Of course, now we will be faced with the issue that it won't work in 8.5.x anymore ...

tr’s picture

tr commented
Status: Active » Needs review

That seems to have worked ... Filing a follow-up issue in core because making this sort of change without a change record is very disruptive to the contributed module ecosystem: #2989254: String changes should get change records

However, I expect that my concerns will be dismissed, as core developers have consistently shown that they don't much care about how their deviations from the documented community process (e.g. requiring change records) affect contributed module developers.

tr’s picture

tr commented

The remaining 6 fails are due to the core change made in #2932462: Add EntityContextDefinition for the 80% use case

I don't have a fix for that yet.

Status: Needs review » Needs work

The last submitted patch, 2: 2989050-2.patch, failed testing. View results

tr’s picture

tr commented
Status: Needs work » Needs review

I just ran the 8.5.x tests to verify what I said in #3: The patch in #2 fixes the 2 test fails going forward in 8.6.x and 8.7.x, but it adds two new fails to 8.5.x.

I don't know how this core policy of bc breaks between minor core versions is supposed to work with contrib - are we supposed to make a new release every time there's a bc break? Or add a core version dependency into our modules (but then, we didn't know for the last release that 8.6.x would introduce a bc break, so we couldn't have known to add a dependency of core < 8.6.x at that time...). Or perhaps we just wait until 8.6.0 is released then publish a new alpha release - still there's no way for us to specify that alpha4 requires 8.5.x while alpha5 requires 8.6.x ... so how is a user supposed to know what all the dependencies are when they try to install or upgrade?

jonathan1055’s picture

jonathan1055 commented
Title: Test pass at 8.5 but fail at 8.6 and 8.7 » Text string detections pass at 8.5 but fail at 8.6 and 8.7
Issue summary: View changes
Related issues: +#2989417: Argument 1 passed to \EntityContext::fromEntity() must implement EntityInterface

OK, I have created #2989417: Argument 1 passed to \EntityContext::fromEntity() must implement EntityInterface to cover the other fails, so this issue can remain concentrated on the text strings.

tr’s picture

tr commented
StatusFileSize
new 2989050-8.patch904 bytes

Here's a new patch for the string change, using the nasty ugly hack that we've used previously in other tests. Hopefully this ensures that 8.5.x tests continue to run while also fixing 8.6.x and 8.7.x tests.

  • TR committed 3685549 on 8.x-3.x 
    Issue #2989050 by TR: Text string detections pass at 8.5 but fail at 8.6...
tr’s picture

tr commented
Status: Needs review » Fixed

OK, I don't like that patch, but I committed it so that our tests would continue to run green in 8.5.x and so that we can eliminate those two string errors in our 8.6.x and 8.7.x tests.

There's a @todo that should help us to find and remove this hack when 8.5.x becomes unsupported.

jonathan1055’s picture

jonathan1055 commented

Thanks for fixing this. Yes the new code looks a bit clumsy, but you have explained it very well, and we have used this method before. We need to have tests passing so that we can detect new failures, so this is a step forward.

Just a minor thing, you could also add
@see https://www.drupal.org/project/rules/issues/2989050
so that all the background info we have in this issue is easily findable by any person reading the @todo.

tr’s picture

tr commented
StatusFileSize
new 2989050-12.patch717 bytes

Good idea.

  • TR committed 009c385 on 8.x-3.x 
    Follow up to issue #2989050 by jonathan1055: Text string detections pass...
jonathan1055’s picture

jonathan1055 commented
Title: Text string detections pass at 8.5 but fail at 8.6 and 8.7 » 'No reaction rule' message uses singular form in 8.5 but plural form in 8.6
Issue summary: View changes

Thanks for adding that.
I have changed the issue title to be specific, to help us when searching later.

  • TR committed d3a3df9 on 8.x-3.x 
    Revert "Follow up to issue #2989050 by jonathan1055: Text string...
tr’s picture

tr commented

The commit in #13 was NOT the patch in #12 that I intended to commit.

I reverted #13, and am now committing the patch shown in #12.

  • TR committed 7c5dcb1 on 8.x-3.x 
    Follow up to issue #2989050 by jonathan1055: Text string detections pass...
jonathan1055’s picture

jonathan1055 commented

Thanks for noticing that. I checked your patch and very often I do check that a commit matches a patch. But for some reason I did not do that this time.

Status: Fixed » Closed (fixed)

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

tr’s picture

tr commented
StatusFileSize
new 2989050-remove-todo.patch971 bytes

Follow up patch to remove the @todo now that Drupal 8.5.x is unsupported, as mentioned in #10.

tr’s picture

tr commented
Status: Closed (fixed) » Needs review

NR so the patch can be tested.

  • TR committed 8ac6bc3 on 8.x-3.x 
    Follow up to Issue #2989050 - remove @todo now that Drupal 8.5.x is...
tr’s picture

tr commented
Status: Needs review » Fixed

Committed #20.

tr’s picture

tr commented
Status: Fixed » Closed (fixed)
jodavidson’s picture

jodavidson commented

Note that version 8.x-3.0-alpha4 that I downloaded on 22 July 2019 still has this issue.

When running the test UiPageTest::testReactionRulePage I received the error

1) Drupal\Tests\rules\Functional\UiPageTest::testReactionRulePage
Behat\Mink\Exception\ResponseTextException: The text "There is no Reaction Rule yet." was not found anywhere in the text of the current page.

Updating the text of line 40 in UiPageTest.php to:
$this->assertSession()->pageTextContains('There are no reaction rules yet.');
fixes the issue.

jonathan1055’s picture

jonathan1055 commented

HI jodavidson,
Yes you are correct. This is because Alpha4 was released on 18 May 2018 (as shown on project front page) but this issue was started after that, on 30 July 2018 (as shown at the top of page) and the first commit was done in #9 above on 1st Aug 2018.

So, yes, Alpha4 still has the fault, but the latest dev release is fixed.