It looks like the patch in #3030295: 'context' deprecated in Rules @Condition annotation missed replacing one use of 'context' with 'context_definitions'. This patch fixes it.

CommentFileSizeAuthor
context-definitions.patch722 bytestr

Comments

TR created an issue. See original summary.

tr’s picture

tr commented
Title: Missed replacing one use of 'context' with 'context_definitions » Missed replacing one use of 'context' with 'context_definitions'
Status: Needs review » Fixed

Committed.

  • TR committed f798a53 on 8.x-3.x 
    Issue #3106468 by TR: Missed replacing one use of 'context' with '...
jonathan1055’s picture

jonathan1055 commented

That's interesting. I would have thought that our test coverage would have found this. But I have just tried it on a main Rules condition and all the tests pass. Also it seems to work interactively just fine with the old 'context' instead of 'context_definition'. Is this because currently we can use both versions, but in D9 we will have to use only 'context_definition'?

tr’s picture

tr commented

I wondered about that too. Perhaps it's because rules_ban is a separate module and we only have Unit test coverage for it - the mocked classes probably don't exercise plugin discovery so annotation isn't parsed etc. I think that even one Functional test, where rules_ban was enabled and a Rule was configured to use it's condition and it's two actions, would probably expose this and maybe other issues with rules_ban.

jonathan1055’s picture

jonathan1055 commented

I think that even one Functional test, where rules_ban was enabled and a Rule was configured to use it's condition and it's two actions, would probably expose this and maybe other issues with rules_ban

That would be a good idea. However, this actual problem does not seem to affect anything in 8.8 and 8.9 (or am I totally off track here?). I reverted back to 'context' for 'node_is_sticky' and ran local tests, which do configure a rule and add the condition, and it all passed. I was also able to use the modified (back to old version of) the condition and the rule fired without any problem. Maybe this would only fail when we eventually get to core 9.0 because we changed it only due to deprecation warnings, not actual errors.

tr’s picture

tr commented

Right, 'context' should still execute properly when used in Conditions - 'context' is just deprecated right now and won't be removed until 9.0, and core has a BC layer to support the old 'context' name in Conditions. In Rules we made the same change from 'context' to 'context_definitions' for RulesActions and Events, but adding a Rules-specific BC layer for these is too much work. Plus we're still in alpha, so there is no expectation for BC between alphas.

However, I would expect to see at least one deprecation warning if a Condition used 'context' in a functional test.

Status: Fixed » Closed (fixed)

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

jonathan1055’s picture

jonathan1055 commented

Ah, I did not get round to posting before the issue got closed.

I would expect to see at least one deprecation warning if a Condition used 'context' in a functional test.

I think the reason is that we don't have any functional tests for Rules Ban, we only have Unit tests, and that warning does not get triggered. In the main rules tests, changing a condition from 'context_definitions' back to the old 'context' causes the deprecation warning to be shown for every functional test loop within ConditionsFormTest. So maybe we should have a similar functional test for Rules Ban? Or can we expand that test and add the rules_ban module conditions to it, and likewise for the ActionsFormTest. That would be simpler than making a new functional test just for Rules Ban.

You can re-open this issue and I'll work on the test here, or it could be in a new issue if you prefer?

tr’s picture

tr commented

A new issue would be best. Thanks.