Port "Flag is flagged" Rules event to 8.x

Here's my first attempt.

I've tried to do a direct port from flag_rules_event_info() in 7.x-3.x, modelled on the plugins currently supplied with Rules, such as \Drupal\rules\Plugin\RulesEvent\EntityInsertDeriver.

1. +++ b/src/Plugin/RulesEvent/EntityFlaggedDeriver.php
   @@ -0,0 +1,98 @@
   +  function __construct(
   +    FlagService $flagService,
   +    TranslationManager $translationManager
   +  ) {
   

   Formatting seems odd here.

2. +++ b/src/Plugin/RulesEvent/EntityFlaggedDeriver.php
   @@ -0,0 +1,98 @@
   +  public static function create(
   +    ContainerInterface $container,
   +    $base_plugin_id
   +  ) {
   

   Here too.

Here's a more comprehensive second try, which adds $user and $flagging members to Drupal\flag\Event\FlaggingEvent and passes the relevant objects in context when events are dispatched.

The included unit test is based on the event tests in the Rules module, such as Drupal\Tests\rules\Integration\Event\EntityInsertTest, which check that the correct event definitions are generated by the deriver. Because of the dependency, I've marked this patch as do-not-test for now.

Moved the test file into the right position for PHPUnit testing (rather than Simpletest).

As indicated on the Rules Github repository, the test can be run with

../vendor/bin/phpunit ../modules/flag/tests/src/Integration

from within the /core directory.

The change looks good, but why do Rules tests need to be run in this way? They don't show up at all with --list-groups.

There are couple of problems with Rules events implementation.

1. Rules is very strict with extracting context from Event, you either have a GenericEvent and pass a list of contexts keyed by context name as second argument, or you have to define public variables on your Event class matching the context name. There is a plan to implement getter support, but not in HEAD right now.

Most likely any non-rules code that uses the FlaggingEvent can use the getFlagging() and chain to get the Flag, account and flaggable entity, but Rules requires you to have them in the event itself. I don't think we should modify the FlaggingEvent just for that.

2. Rules events in Drupal 7 had a per-entity-type and per-flag and per-action combination event name. We actually have 2 things that matter, it's the flag and the action, as flag can have only one entity type attached to it. However one entity type can have multiple flags, and in previous implementation the deriver was based on entity type, which would store only one flag event per entity. Also, because rules events require to have multiple derivatives per same Symfony event, we can't really use the native FlaggingEvent, as it's name is purely flag.entity_flagged, and we need flag.entity_flagged.[derivative_id].
I had to catch the native FlaggingEvent sent by flag and use GenericEvent to pass a specific rules event and proper context.
I also switched from flag.entity_flagged.[entity_type] to flag.entity_flagged.[flag_id] because of the above mentioned reasons.

3. Added functional kernel tests to test event interaction and contexts availability.

This patch includes both flagging and unflagging actions as they are very very similar and based on same deriver base class.

A year later ... there appears to be lots of good work here which should not go to waste..

I have made only trivial changes here to keep up with cores changing coding standard rules

I hope to get to a more detailed review before the weekend.

I am not a Rules user ... so my comments should be treated as I am just being a helpful bunny and trying to break log jams in the flag issue queue.

Steps to reproduce.

drush en flag rules
and visit 'admin/config/workflow/rules/reactions/add'

I get the following error

[27-Jul-2017 15:13:28 Europe/Berlin] Error: Call to undefined method Drupal\flag\FlagService::getFlags() in XXX/drupal/modules/flag/src/Plugin/RulesEvent/EntityFlagEventDeriverBase.php on line 59 #0 XXX/drupal/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php(101): Drupal\flag\Plugin\RulesEvent\EntityFlagEventDeriverBase->getDerivativeDefinitions(Array)

1) So yeah in a year methods in the FlagService have been updated - breaking this patch
2) I think we need to identify why the new test did not execute this aspect of the code.

I retested the patch in #10 with the testbot to confirm it still works.

I manually tested the flag.entity_flagged.[flag_id] and flag.entity_unflagged.[flag_id] events in the Rules UI - with a minor change I corrected the error reported in #10 (getFlag() had been renamed to getAllFlags()). I also fixed the tests.

[Note that the unit test did not detect that getFlags() no longer exists because the test mocked the return of getFlags(). This sort of thing is one of the basic problems with unit testing - you're supplying all the 'expected' input, but when the larger system that you're not testing changes, you still supply the same input.]

To manually test the rules events, I first created a flag for my article content type. I then added a reaction rule to trigger an action (system message) when an article was flagged, and added another rule to trigger a different action when an article was unflagged.

When I subsequently flagged an article, I saw the system message displayed. Likewise, when I unflagged the article, I saw a different system message.

I also repeated this on a D7 site to make sure the D8 behavior I was testing was exactly the same as how it worked in D7. It was.

I conclude that this patch is basically good to go. I think the only way to expose any other possible problems is to commit the patch and let people start using the code.

So here's a new patch which uses getAllFlags() and fixes some minor coding standards issues. Note that if this is committed, it also fixes #2540034: Port "Flag is unflagged" Rules event to 8.x.

Actually, I noticed that the new tests introduced in #8 aren't running on the testbot. For two different reasons:

FlagEventsTest was not getting run by the testbot because the testbot now requires unit tests to be found in MODULE/tests/src/Unit. I fixed this by moving the test to where it should be. I did have to change the namespace statement, a use statement, and an internal filepath to accommodate the new location.

The other new test (FlagRulesEventIntegrationTest) was also not getting run, because the namespace declaration was wrong - it was declared in the rules namespace rather than the flag namespace, so PHPUnit did not see it (it's only looking for tests in the flag namespace). A simple change of namespace fixed this.

So I rolled a new patch which is the same as #11 but includes the above fixes to the tests.

Hopefully the test now runs and passes like it does locally for me.

Testbot reports "No tests found", but they actually were found this time - that's good - however they weren't run because the base class for the Rules tests was not found. The only way to fix this is to add a testing dependency on the Rules module, and that would have to be committed to the Flag project BEFORE the tests in this issue could be run again. (That's because test dependencies are calculated before the patch is applied, so you can't patch the dependencies in the same patch as the other fixes.)

This is what needs to be added for the test dependency:

diff --git a/flag.info.yml b/flag.info.yml
index 99e76a1..b1de0fd 100644
--- a/flag.info.yml
+++ b/flag.info.yml
@@ -3,4 +3,6 @@ description: Create customized flags that users can set on entities.
 core: 8.x
 type: module
 package: Flags
+test_dependencies:
+ - rules:rules
 configure: entity.flag.collection

Patch for the test dep update only.

Retesting now that the test dependencies are updated.

Yes, the test failed, but it found it. :-D

Looks like we have a deprecation problem:

\Drupal\Core\Routing\Enhancer\RouteEnhancerInterface is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, you should use \Drupal\Core\Routing\EnhancerInterface. See https://www.drupal.org/node/2894934: 1x
1x in FlagRulesEventIntegrationTest::testEntityFlaggingEvent from Drupal\Tests\flag\Kernel

Looks like we have a deprecation problem:

Yes, this is actually a core problem. It looks like your project hadn't had a branch test run since 8 Nov, so this is your first encounter with the core issue.

#2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code
#2607260: [meta] Core deprecation message introduction, contrib testing etc.
#2922757: Replace deprecated RouteEnhancerInterface, then remove @group legacy

In short, as of 10 Nov, core changed 8.5.x and now if a core function is marked @deprecated it will cause contrib tests to fail if the contrib module uses that @deprecated function or even if the contrib module doesn't use the deprecated function (like in the case of this patch) but depends on a module (Rules) that does use a deprecated function. There is no change notice for this yet.

The immediate consequence of this core bug is that whenever core now introduces a @deprecated function, which it will continue to do in 8.5.x and beyond, any contrib that uses the old function will IMMEDIATELY start to fail the 8.5.x tests. (I keep saying "contrib" because core code is itself expressly excluded from generating test fails due to @deprecated usage - this problem only hits contrib.)

The current suggestions from core developers are that contrib shouldn't be using "development" (currently 8.5.x) for tests, but instead should be using "pre-release" (currently 8.4.x). You can change that under the automatic testing tab on your project page if you want. I don't see this as a long-term fix though.

I just re-tested the patch in #12 and you will see above that it now runs and passes with 78 passes in 8.4.x. This is due to the test_dependency you committed in #14. Thanks for that! It still fails in 8.5.x because of the core issue.

Going forward, this is going to continue to be a problem until core gives us deprecation *notices* instead of test fails. Your project can and will be thrown into test failure and all the patches in your issue queue will start failing when core marks another function that you use @deprecated, and it will require your immediate attention to fix it. IMO this is extremely disruptive and NOT at all what @deprecated is supposed to mean. (@deprecated means: The function still exists and will continue to work until Drupal 9.0.0, at which point it will be removed. You should not introduce any new usage of a @deprecated function, and you should replace your current usage of @deprecated functions at your convenience but before Drupal 9.0.0.)

I think I can fix the patch by marking the new tests with @group legacy - this only works with phpunit tests, not simpletests, but the new tests are phpunit tests so it should work. Here's a new version to see if this strategy will work. Regardless, your feedback in the above issues would be appreciated, as you are the maintainer of a major module which is affected by this core change.

Sorry, forgot to pull your changes from #14 before I re-rolled the patch. Ignore the patch in #19, here's the correct one:

Sigh. "AlreadyInstalledException" and "Connection refused" errors are caused by the testbot malfunctioning. I guess I'll have to wait until that gets resolved then try the tests again. The tests in #21 run green locally.

Issue filed in testbot queue #2926245: testbot malfunctions

@TR thanks for posting all your comments .. an interesting read ... started testbot up again .. all green.

It looks good to go. I would like someone more familiar with Rules than myself to mark it RTBC, though.

You can manually test the functionality by following the steps I outlined in comment #11 above.

I re-rolled the patch because it no longer applied after #2877902: Coding standard changes went in.

Hi,
This looks good. Manual testing shows that you must be doing this pretty much right.

From #11

Note that the unit test did not detect that getFlags() no longer exists because the test mocked the return of getFlags(). This sort of thing is one of the basic problems with unit testing - you're supplying all the 'expected' input, but when the larger system that you're not testing changes, you still supply the same input.

To give extra confidence in these scenarios, have you considered adding a functional test to verify the UI? You could do something like Rules does in rules/tests/src/Functional/ConfigureAndExecuteTest.php. You could also add functional tests to verify that the rules action is performed, such as Scheduler does in scheduler/tests/src/Functional/SchedulerRulesActionsTest.php

Also I noticed in the patch the @see url is @see https://www.drupal.org/project/scheduler/issues/2924353 which is the Scheduler issue for this problem. You probably used that as an example to copy and paste, but maybe it would be better to use @see https://www.drupal.org/project/rules/issues/2922757 which is the Rules issue.

Jonathan

Re-roll of patch #27 against the current HEAD of Flag.
The only change was to remove the @legacy documentation tags; they are no longer needed because #2922757: Replace deprecated RouteEnhancerInterface, then remove @group legacy is long-since fixed.

Note: The PHP 5.5/D8.8.x tests fail because D8.8 does not support PHP 5. See #3061979: Fix automated test configuration.

There are three test failures in patch #29. One of them, AjaxLinkTest, is a branch failure (fails on the bare branch, without this patch) which and thus must be fixed separately. See #3038818: AjaxLinkTest is broken

The other two failures are because of Rules and are fixed by using Rules 8.x-3.x-dev - when Rules releases a new version these errors will go away. Or Flag could test against the -dev version of Rules, but that's more trouble than it's worth.

There are three test failures in patch #29. One of them, AjaxLinkTest, is a branch failure ...

The other two failures are because of Rules and are fixed by using Rules 8.x-3.x-dev - when Rules releases a new version these errors will go away.

Rules now has a new release.

I retested #29 and as you can see that the two Rules-related failures have gone away as promised. (See https://www.drupal.org/pift-ci-job/1397416). The AjaxLinkTest failure remains and should be fixed by #3038818: AjaxLinkTest is broken

Is there any reason this patch shouldn't be committed now?

Good news - Flag is going to be maintained in 8.x #3080565: Is flag still being maintained?

I triggered another retest on #29 now that #3038818: AjaxLinkTest is broken is fixed, and all the tests pass.

I test out the patch from #29. The action is working.

I've only test it with creation of content. not with entity referred with the flag itself.

Is there any reason this patch shouldn't be committed now?

Having issues with this patch. Not sure if the problem lies with the patch or with what I've done / not done.

Applied the patch, updated the DB. Got error:

TypeError: Argument 1 passed to Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer::renderBarePage() must be of the type array, boolean given, called in example.org/web/core/modules/system/src/Controller/DbUpdateController.php on line 196 in Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer->renderBarePage() (line 73 of example.org/web/core/lib/Drupal/Core/ProxyClass/Render/BareHtmlPageRenderer.php) #0 example.org/web/core/modules/system/src/Controller/DbUpdateController.php(196): Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer->renderBarePage(false, Object(Drupal\Core\StringTranslation\TranslatableMarkup), 'maintenance_pag...', Array) #1 [internal function]: Drupal\system\Controller\DbUpdateController->handle('admin', Object(Symfony\Component\HttpFoundation\Request)) #2 example.org/web/core/lib/Drupal/Core/Update/UpdateKernel.php(115): call_user_func_array(Array, Array) #3 example.org/web/core/lib/Drupal/Core/Update/UpdateKernel.php(76): Drupal\Core\Update\UpdateKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request)) #4 example.org/web/UPDATE.PHP(28): Drupal\Core\Update\UpdateKernel->handle(Object(Symfony\Component\HttpFoundation\Request)) #5 {main}.

Flushed caches. Updated DB again, no error this time. Flushed caches.

Created a test content type and a test flag. Tried to create a rule to fire when the flag is changed. Rules & flag behavior seems the same as before applying the patch.

See 6 attached screenshots.

Hi, Using D8.8.2, rules 8.x-3.x-dev, flag 8.x-4.x-dev
and tested patch #29 but I have errors and problems.

When I create a new rule:
I see the new events ('a node has been flagged/unflagged' for each flag created) but gives me this error when saving the rule:
TypeError: Argument 2 passed to Drupal\rules\Engine\RulesComponent::addContextDefinition() must implement interface Drupal\rules\Context\ContextDefinitionInterface, array given, called in C:\localhost\d8-8-2-dev1\modules\contrib\rules\src\Engine\RulesComponent.php on line 176 in Drupal\rules\Engine\RulesComponent->addContextDefinition() (line 146 of C:\localhost\d8-8-2-dev1\modules\contrib\rules\src\Engine\RulesComponent.php)

With the patch installed, I cannot unflag every flag on nodes, having this error:
ParseError: syntax error, unexpected ';' in Composer\Autoload\includeFile() (line 17 of C:\localhost\d8-8-2-dev1\modules\contrib\flag\src\Event\UnflaggingEvent.php)
Removing the part of the patch related to the file UnflaggingEvent.php:
- protected $flaggings = [];
+ protected $flaggings;
I can again unflag every node.

Don't if you are able to set a rule with the event 'a node has been flagged/unflagged',
hope someone can support me.
thanks

@alternativo:

ParseError: syntax error, unexpected ';'

There is absolutely no syntax error in the patch. That would be impossible - the test bot would catch it immediately, and you can see from the above test results that didn't happen. Likewise, the lines you show are perfectly fine. I don't know what happened on your site to corrupt your files but it's not a problem with the patch.

However, the patch does need to be re-rolled because of the many major changes and improvements that Rules has made over the past year, and because of the changes that Drupal core has made to the context code over the past year.

@JWBrown:
You didn't post an export of your Rule, so it's impossible to see how you've configured it. Those images don't show what condition you're using, they just show the list of conditions - I don't see how that helps us understand what you've done.

Those errors don't seem to be related to Rules or Flag at all.

Thanks for the fast reply TR.
I agree with you, there is no sintax error, just the []...I will try a clean installation of rules and flag, and write my patch tests here.

When patch, getting this log with latest flag 4.0-beta2:

patch -p1 < 2540032-29.patch
patching file flag.rules.events.yml
patching file flag.services.yml
Hunk #1 FAILED at 26.
1 out of 1 hunk FAILED -- saving rejects to file flag.services.yml.rej
patching file src/Event/FlagEventBase.php
patching file src/Event/FlaggingEvent.php
patching file src/Event/UnflaggingEvent.php
patching file src/EventSubscriber/RulesEventSubscriber.php
patching file src/Plugin/RulesEvent/EntityFlagEventDeriverBase.php
patching file src/Plugin/RulesEvent/EntityFlaggedDeriver.php
patching file src/Plugin/RulesEvent/EntityUnflaggedDeriver.php
patching file tests/src/Kernel/FlagRulesEventIntegrationTest.php
patching file tests/src/Kernel/FlagServiceTest.php
Hunk #1 FAILED at 165.
1 out of 1 hunk FAILED -- saving rejects to file tests/src/Kernel/FlagServiceTest.php.rej
patching file tests/src/Unit/Integration/Event/FlagEventsTest.php

@muaz91: Yes, that's what I said in February. It would help if you could re-roll the patch to fix those very minor problems ... pointing out problems that have already been mentioned isn't very useful.

Fixed new event dispatcher signature in d10.

Oops, messed a bit with patch