Port "Flag a [entity type]" Rules action to 8.x

working on this at the drupal dev days Montpellier

Will try to take over ;)

Still working on it: you can watch the progress on: flag fork.
Current state: skeleton with 2 Unit tests.

Can I just make sure your github clone is a clone of Flag from the d.org repository, and not from the old hithub repo that's out of date?

@Joachim

It's a clone from git://git.drupal.org/project/flag.git.

Current state: 2 unit tests shows that the action works.

Prepared a patch for the issue 2470661 and 2470665 with patch for rules to be able to reuse the RulesEntityIntegrationTestBase

Looks like a great start!

Also: wish I could be at Drupal dev days myself! :(

1. +++ b/src/Plugin/Action/Base.php
   @@ -0,0 +1,62 @@
   + * Contains the \Drupal\flag\Plugin\Action\Base class.
   

   Should be of the form " * Contains \Fully\Qualified\Namespace\And\NameOfTheClass." (And yes, Flag module doesn't live up to this everywhere...)

2. +++ b/src/Plugin/Action/Base.php
   @@ -0,0 +1,62 @@
   +   * @var FlagService $flagService
   

   This needs to be fully qualified, and also needs a description.

3. +++ b/src/Plugin/Action/Base.php
   @@ -0,0 +1,62 @@
   +   * @param array       $configuration
   +   * @param string      $plugin_id
   +   * @param mixed       $plugin_definition
   +   * @param FlagService $flagService
   

   These shouldn't be tabbed, and need descriptions.

4. +++ b/src/Plugin/Action/Flag.php
   @@ -0,0 +1,54 @@
   +class Flag extends Base {
   

   Hmm yeah definitely not keen on this class name!

5. +++ b/src/Plugin/Action/UnFlag.php
   @@ -0,0 +1,54 @@
   +    return $this->t('UnFlag entity');
   

   Is that a label? We always write 'unflag', without internal capitalization.

@joachim: should I rename all occurences of 1 in this patch? (also in unchanged files from this patch?)

Did rework patch with all comments on the code review. It is now more like in drupal core. Thanks much for the time to review it.
I did not refactor all other comments as it probably belongs into a separate issue.

Added rerolled patch.

Short question: for the attribution it would be nice if you could mention katzilla as well, as we did 2470661 and 2470665 in pair programming. Does that work?

please don't merge... looks like upstream did change execute to call doExecute

> Short question: for the attribution it would be nice if you could mention katzilla as well, as we did 2470661 and 2470665 in pair programming. Does that work?

Sure, though it would help if they made a comment here as then it's just a ticky box in the commit message generator :)

yay - here is my comment :) Thank you @pjezek for mentioning!

Here is the new patch with refactored for doExecute which was added recently in rules module.

Looks like rules has moved from using the Action plugin to using RulesAction, so that needs to be updated.

I took a stab at updating the patch from #26. Is there anyone else familiar with Rules who could review it, or have I just shot myself in the foot?

These are all passing on my local; I have no idea why the RulesEntityIntegrationTestBase class isn't being found? Is there something I'm missing that's different between my local code and jenkins?

On D7 at least, you need to tell the test system of any contrib modules you want -- by default, your patch is tested with core + this module + your patch and that's it. I don't know how it works on D8...

@joachim: That goes into the test_dependencies in the info.yml file, right? I thought just adding the rules dependency would have done it, but maybe I need to add rules_test too. Trying with this new patch.

Last try before I head home.

Last attempt and then I'm giving up and letting someone else figure it out because this should not be this difficult so clearly I am missing something.

+++ b/src/Tests/Integration/FlagIntegrationTestBase.php
@@ -0,0 +1,71 @@
+use Drupal\Tests\rules\Integration\RulesEntityIntegrationTestBase;

If this is from Rules module, then the namespace looks wrong -- aren't they always of the form Drupal\MODULE\STUFF?

Also, is the a reason why Rules has its tests further namespaced with 'Integration'? Do we have to do that too?

For whatever reason, rules has its tests outside the src directory (rules/tests), so they need to be namespaced like that. It's really weird and while I'm sure they have a good reason for it, I'm not a fan.

I don't think we need to have any particular folder structure, I just didn't want to chuck all the rules tests in with everything else, to make them easier to find. I can totally move the tests up to the main Tests folder, though, if you want me to!

The critical line is buried in

https://dispatcher.drupalci.org/job/default/24385/console

22:00:34 cd /var/www/html && sudo -u www-data php /var/www/html/core/scripts/run-tests.sh --url http://localhost/checkout --dburl mysql://drupaltestbot:drupaltestbotpw@drupaltestbot-db-mysql-5-5/jenkins_default_24385 --color --keep-results --color --concurrency 31 --sqlite /var/www/html/results/simpletest.sqlite --php /opt/phpenv/shims/php --directory modules/flag
22:00:34 Command created as exec id 5f0f566c
22:00:35 PHP Fatal error: Class 'Drupal\Tests\rules\Integration\RulesEntityIntegrationTestBase' not found in /var/www/html/modules/flag/src/Tests/Integration/FlagIntegrationTestBase.php on line 14
22:00:35
22:00:35 Fatal error: Class 'Drupal\Tests\rules\Integration\RulesEntityIntegrationTestBase' not found in /var/www/html/modules/flag/src/Tests/Integration/FlagIntegrationTestBase.php on line 14

Solution seems simple... I will get to this tonight...

PS We are using Jenkins now!... that is really good news.

Three things to talk about :-

1) The AutoLoading quagmire

Something that could not get sorted out in 5 mins on a Friday night sorry!

This may need a patch in rules before we can proceed. So I am marking this issue postponed :(

This from #44

If this is from Rules module, then the namespace looks wrong -- aren't they always of the form Drupal\MODULE\STUFF?

My answer is unsatisfactory - No that is perfectly correct according to this Drupal8 psr-4 autoloading guide..

https://www.drupal.org/node/2156625

BUT the actual implementation is restrictive ... maybe by design I haven't worked that out yet.... or else it is a core testing bug......

IF you use the alternate convention for RulesEntityIntegrationTestBase and RulesIntegrationTestBase then they become available to other modules.

To demonstrate this, I have made a minimal RULES patch - minimal in that it breaks rules but makes this flag patch work :)

2) Coding standard fixed.

I have passed the patch through phpcs and fix lots of newline issues etc

3) Dependancy Issue

In order for the test to pass we have introduced a dependency of rule in the flag.info.yml. I think this is wrong ... and I am looking for suggestions for ways to make those tests optional, or a least not make the whole module depend on rules.

I can think of plenty of flag using applications where in production environments pulling in the rule module would be inappropriate.
There is a security mantra
"Each developer owns the bugs in the modules they enable" ... and the enabled modules should be kept to a minimum.

> Drupal\Tests\rules\Integration\RulesEntityIntegrationTestBase;

So according to the PSR-4 autoloading guide you linked to, that class doesn't have the right format.

It should be Drupal\rules\Tests\etc.

> In order for the test to pass we have introduced a dependency of rule in the flag.info.yml. I think this is wrong ..

Agreed. On D7 there was a 'test_dependencies' property in the info file, but I can't find the .info spec for D8 anywhere.

1) Ah, my reading of that guide is different....

Under the section "Namespace resolution"

looking at the final row of the table in the modules sub section.

we have this entry base namespace

Drupal\Tests\$modulename\

maps to base directory

modules/$modulename/tests/src/

"Drupal\Tests\rules" maps to directory "modules/rules/tests/src/"

2) Regarding the testing dependencies loss of functionality, I have hit a brick wall and need advice.

The conventional wisdom is that things are sandboxed and instead of the D7 test_dependencies option
the D8 alternative is to populate the "modules" property of a class that extends KernelTestBase
and that pull dependencies into the sandbox.

I had debugged the problem properly, just did not describe it in the previous issue..... I now think that it is a core testing bug.

Here is my test command line.

php core/scripts/run-tests.sh --color --die-on-fail --url http://dev.drupal.co.uk:7888 --verbose --file ./modules/flag/src/Tests/Integration/Action/ActionFlagTest.php

and the function in run-test.sh where thing go phut is

simpletest_script_get_test_list()

on this line

  if (is_subclass_of($namespace_class, '\Drupal\simpletest\TestBase') || is_subclass_of($namespace_class, '\PHPUnit_Framework_TestCase')) {
    $test_list[] = $namespace_class;
   }

The standard class loader is passed to the TestRunnerKernel and things proceed as you expected - the problem is simpletest_script_get_test_list()
is outside of that path and so the if statement blow up because php fails to resolve the namespaces and can't process the is_subclass_of() calls.

+++ b/src/Tests/Integration/FlagIntegrationTestBase.php
@@ -0,0 +1,71 @@
+
+namespace Drupal\flag\Tests\Integration;
+
+use Drupal\rules\Tests\RulesEntityIntegrationTestBase;
+
+/**
+ * Base class that holds common used methods for testing flag features.
+ */
+class FlagIntegrationTestBase extends RulesEntityIntegrationTestBase {

Yes, you can not extend from a base class from an optional dependency, that also wasn't possible in 7.x

Simpletest AFAIK uses reflection to parse test classes and that fails hard if that base class doesn't exist.

There is currently no solution to this problem that I'm aware of.

Also, you're mixing simpletest and phpunit here. The rules class you're extending is a unit test. Which live in tests/src (Drupal\Tests\$module). Your own class is now in src/Tests (Drupal\$module\Tests), which is where simpletest s live.

You must move it to the correct namespace and folder.

Also...

The conventional wisdom is that things are sandboxed and instead of the D7
test_dependencies option
the D8 alternative is to populate the "modules" property of a class that
extends KernelTestBase
and that pull dependencies into the sandbox.

There are 3 separate things:

a) test_dependencies in .info.yml. This is a testbot/DrupalCI/d.o parsing specific extension, Drupal core does not use or read that anywhere. It's only purpose is to allow d.o to make those optional modules available on test environment

b) "@dependencies $module" in the class annotation (dependencies key in getInfo() in 7.x). This is a Drupal Core/simpletest thing that tells it to ignore those tests if the given dependency is not around. They then simply don't exist in the list of tests. There are two limitations with this: first, it doesn't work with the base class problem above, as that already fails on parsing that information. Second, it is currently broken with --files in D8, which is what the testbot is using. See #2383395: `run-tests.sh --file` disregards missing dependencies

c) What kind of modules a specific test actually enables. This is the only thing that actually matters when a test is executed. Conceptually exactly the same as in D7, just the way it is defined changed (class property vs. passing it to setUp()). Obviously this only works if the modules are actually present, which is what a) and b) are for.

Thanks for the information .. I will have some more spare time ... latter in the week.

Coming back to this now ....

From #50

Yes, you can not extend from a base class from an optional dependency, that also wasn't possible in 7.x

I see two paths forward

1) We keep copies of

RulesIntegrationTestBase and RulesEntityIntegrationTestBase in the flag namespace

which, while possible, makes me grumpy and mumble maintenance hazard under my breath. :)

2) I went looking for other examples of tests that solve this cross module problem.

here is an example of tests that mixes the rules and ban module.

\Drupal\Tests\rules\Integration\Action\BanIPTest which like our tests extends RulesIntegrationTestBase.

So my next proposal let's move the plugin to flag, BUT keep the testing in the rules module. So we can keep the test techniques to solve these problem in the same place [ Ahem or keep fudges localized. :) ]

[ I am looking for ways to disagree with this point to view..... is the long term plan for the rules module to move the plugins and test into their respective modules -- if so what I am saying is wrong! ]

Anyway more tomorrow.

I have created a postponed issue for the rule module where we can add out testing.

#2602314: Add testing for Flag plugins

Changes made to this patch.

1. flag.info.yml: Removed the "rules" dependency.
2. Removed the testing.

1) We keep copies of

RulesIntegrationTestBase and RulesEntityIntegrationTestBase in the flag namespace

which, while possible, makes me grumpy and mumble maintenance hazard under my breath. :)

Indeed. Not going to happen, would cause grumpiness here too.

> keep the testing in the rules module.

Sadly, I don't think that's viable either. I can't imagine the Rules maintainer being ok with that. And then, how do we run the tests that are in Rules on patches in this issue queue?

> There are two limitations with this: first, it doesn't work with the base class problem above, as that already fails on parsing that information. Second, it is currently broken with --files in D8, which is what the testbot is using. See #2383395: `run-tests.sh --file` disregards missing dependencies

So am I right in thinking this is a core problem?

Is it simply the case that a PHPUnit test class from contrib module Foo can't be inherited by module Bar if module Bar doesn't declare Foo as a module dependency?

Is it simply the case that a PHPUnit test class from contrib module Foo can't be inherited by module Bar if module Bar doesn't declare Foo as a module dependency?

Yes, basically.

I believe there is one trick that you can try that might work, I've used that before in 7.x IIRC.

Basically, you put the following at the top of each file:

if (!class_exists('the\parent\class')) {
  return;
}

Then it will stop parsing it there and silently ignore those files. I haven't tried if that still works in 8.x with the completely rewritten discovery, though.

Ok let's try that trick then. (Fix your tests with this one neat trick... ;)

If that doesn't work, then it's a core bug. Lots of modules will want to use base test classes from modules they don't depend on -- any module implementing Rules support for starters.

Though one other fix occurs to me: we move the Rules support into a submodule. Then Rules is a hard dependency.

It is good to have test and plugin in the same module....I am glad it looks like we have a solution.

thanks Berdir

so #47 becomes the patch to work from.

That patch altered a travis file and #2594275: remove travis stuff removed that file.

This new patch is just a reroll.

It is good to have test and plugin in the same module....I am glad it looks like we have a solution.

thanks Berdir

so #47 becomes the patch to work from.

That patch altered a travis file and #2594275: remove travis stuff removed that file.

This new patch is just a reroll.

This looks a lot like #2605664: [Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits, where I'm having an issue like this with the testbot trying to factor a trait.

If it is, by all means add your information to that issue.

Rules does its testing entirely via Travis, but I suspect that this issue might affect Rules also. I'll do some tests to see if that's true.

@emclaughlin #33:

For whatever reason, rules has its tests outside the src directory (rules/tests), so they need to be namespaced like that. It's really weird and while I'm sure they have a good reason for it, I'm not a fan.

That's actually standard for PHPUnit tests. They're in the Drupal\Tests\ namespace, and MODULE_BASE/tests/src is actually the right place for them to be.

Tests like Drupal\module\Tests\YadaYadaTest.php are SimpleTests.

The problem here is that the test runner's notion of where PHPUnit test-related items (tests, base classes, etc.) is extremely restricted. Only items in the Drupal\Tests\module\Unit and Drupal\Tests\module\Functional namespaces will be "seen" by the run-tests.sh autoloader; every other item under Drupal\tests\module will give you a "class not found" error. This is not a PHPUnit issue per se; it's a problem with the loader code in Drupal\simpletest\TestDiscovery. Run your tests using ../vendor/bin/phpunit and they will very likely work; but use php scripts/run-tests.sh and the loader will crap out as you folks are seeing.

Drupal\Tests\rules\Integration\RulesIntegrationTestBase is not in one of these two blessed namespaces, so to run-tests.sh, the class is literally invisible.

The Rules people get around this by not using the official Drupal testbot at all; they've moved their sources to github, and they use a Travis-based test runner that runs plain vanilla phpunit.

Ideally, this will get fixed in the testbot, since once projects like Rules get frustrated with the testbot and move out of the standard work flow, they no longer are even aware of issues like this. I think that's the priority. But since this problem is preventing you from doing your Rules 8.x-3.x port, you may want to raise this issue on the Rules issue queue on d.o.

The Rules people get around this by not using the official Drupal testbot at all; they've moved their sources to github, and they use a Travis-based test runner that runs plain vanilla phpunit.

Thanks for that observation ... it explains alot.

I was wondering why there is a whole family of test like \Drupal\Tests\rules\Integration\Action\BanIPTest - do not show up on "admin/config/development/testing"

"Testbot friendly" is a good name for it!

I think we should postpone on #2605664: [Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits and #2609816: Rules tests should be "testbot friendly"

Agreed on postponing.

Could someone maybe comment on #2609816: Rules tests should be "testbot friendly" to explain how that's affecting contrib modules that want to have Rules support?

The workflow in my head, which now seems a bit weak :-

Resolve the "testbot friendly" kinks in the rules module ... no matter how small..
Once the dust had settled we would have a template from which to make our test files.

I think there will be a few more issues created and resolved before we have our template.... so yeah #2609816: Rules tests should be "testbot friendly" seems minor.

hey, anything we can do to help here?

would like to check if we can close #2602314: Add testing for Flag plugins as well

#2605664: [Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits has bogged down in controversy.

How about going with #57 for now:

move the Rules support into a submodule. Then Rules is a hard dependency.

it'd be easy enough to move the submodule back into the main module later?

Agreed. Let's go with a submodule.

Any news about this issue?

Tried 57 against 8.4-dev without any luck.

#2605664: [Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits eventually got committed. See https://www.drupal.org/node/2848889 for the change record explaining the outcome of the monster issue. Let's try the bot here.

Hoping for updates to the underlying issue of getting better Flag->Rules integration... And in particular with flagging nodes/entities via Rules. I'm having trouble finding any recent answers or documentation.

I've looked at several of the related issues (this issue, port Flag is flagged, and the [meta] issue, and most seem to trail off a year or two ago?

I do see here in this issue the notion of a submodule, did that ever happen?

Is the "port flag is flagged" patch supposed to help much?

Is this already going and we just need better documentation?

Is there somewhere else (github, etc) that this is being worked on, or have things just gone quiet?

In particular, I'm looking for the functionality in this issue: the ability to create Rules that Flag nodes (or entities) on behalf of users. I did find this stack exchange article to Flag nodes through a custom module, but I'd really prefer to get it into the UI. I'm sure we all would.

Note: I'm happy to pitch in with documentation or try at a patch, or to possibly sponsor some development in this regard. I use those modules A LOT, and would definitely love to see better implementations in D8.

Another note: I do see the action(s) in the dev versions, but no documentation about them, and I can't seem to get them to work with what I've (so far) sussed from the modules themselves.

I've been the sole Rules maintainer for more than a year now, and this is something I'd like to see happen (as I stated in #2424465: [Meta] Implement Rules 8.x Support). But I can't do this by myself - I need a Flag maintainer to co-operate to get this done. I have all the code ported to D8 and working on my local, but my initial patch (#2540032-27: Port "Flag is flagged" Rules event to Drupal 10+) has not been committed so I am reluctant to spend the effort to try to get the rest of the integration committed.

Hello,
I'm using D8.8.2, rules 8.x-3.x-dev, flag 8.x-4.x-dev
and tested patch #58 but no way to make flag action on node working.

I see the new actions 'flags/unflags the specifies the entity', the system tries to perform flag action (connect to 'content is viewed' event), but i did not manage to select the right entity to put in Drupal\flag\Plugin\RulesAction\Flag::doExecute()
Error:
TypeError: Argument 1 passed to Drupal\flag\Plugin\RulesAction\Flag::doExecute() must be an instance of Drupal\Core\Entity\Entity, string given in Drupal\flag\Plugin\RulesAction\Flag->doExecute() (line 43 of C:\localhost\d8-8-2-dev1\modules\contrib\flag\src\Plugin\RulesAction\Flag.php)

I tried with different entities...and also to set a condition 'entity is of type' and 'Entity is of bundle' to make entity apper in data selector, but no way...seems we are near to make it work, but don't know how to solve

Any suggestions?

Thanks to all for your work ;)

must be an instance of Drupal\Core\Entity\Entity, string given

That means you're probably using the direct input mode and typing the name of your entity variable (a string) instead of using the data selector mode and passing it an entity object.

Regardless, this patch needs a serious re-roll because Rules has had massive changes/improvements in the 4+ years since that patch was first posted. Specifically, Rules will now prevent you from making the above mistake if the action is coded correctly.

As I said in #80, the maintainer of this module doesn't seem interested in getting this feature ported to D8. The first step is to get the events committed, as that is the easy part - if there is no interest in doing that then there's no sense in wasting time on the conditions and actions.

Yes, @TR I read...and I'm sad about that...Flag (and Rules) are great modules.

I tried with data selection, but no way to catch the flag entity on my 'annuncio' content.
I also installe rules token to have the token for the flag in the content, but it gives me only the flag link and count.

I don't know how to select the right entity in data selection:if I start with node, I cannot find the flag entity inside, maybe because it's not a field...even if I select the option in flag settings, and I have flag field in manage disply of content type.
Here's my rule exported (this time with node selected as entity with data selection mode):
uuid: c4e93c0c-5982-4f9b-ad28-2eb66f26c9b2
langcode: it
status: true
dependencies: { }
id: flag_rule_2
label: 'flag rule 2'
events:
-
event_name: 'rules_entity_view:node--annuncio'
description: ''
tags: { }
config_version: '3'
expression:
id: rules_rule
uuid: 4e7c64ab-2ae6-42e8-beb4-ca8f454b063f
weight: 0
conditions:
id: rules_and
uuid: 79d8e383-766d-477a-a031-460a4d513625
weight: 0
conditions: { }
actions:
id: rules_action_set
uuid: 4b62e6ec-7c70-4f87-b3f9-9eaf67562536
weight: 0
actions:
-
id: rules_action
uuid: 2de4403a-081d-4336-8fd2-24aff3d3b1ac
weight: 0
context_values:
message: Flag!
type: status
repeat: false
context_mapping: { }
context_processors:
message:
rules_tokens: { }
type:
rules_tokens: { }
repeat:
rules_tokens: { }
provides_mapping: { }
action_id: rules_system_message
-
id: rules_action
uuid: 4b92b07a-72ff-4674-971f-a22edf19c3ff
weight: 0
context_values:
flag: read_flag
context_mapping:
entity: node
context_processors:
flag:
rules_tokens: { }
provides_mapping: { }
action_id: flag_action_flag

5 years without any activity. I think we can close it as outdated. Please reopen if needed.