Add relationship to flagged entities when Flagging is base table

Each flag type has a specific target entity type. If you just switch to loop over flag types and get their entity type ID (and check if you haven't already defined that, as there are possibly multiple flag types with the same entity type) then you should only get those that matter.

And AFAIK, we already clear the views cache when changing flag types, so that should work too.

> If you just switch to loop over flag types and get their entity type ID (and check if you haven't already defined that, as there are possibly multiple flag types with the same entity type) then you should only get those that matter.

Yup.

+++ b/src/FlaggingViewsData.php
@@ -20,6 +20,32 @@ class FlaggingViewsData extends EntityViewsData {
+        'id' => 'standard',

Also, the relationship handler should let you filter on the actual flag, the same way that the Flaggable -> Flagging relationship does.

That's a good start however joining a string and an int is affront to all scalability gods whether comment does it or not.

Bother!

See node_update_8001() for an example on how to add a new base field.

Why do we need two entity ID fields?

@chx and @ksenzee we don't need new basefield at all we have all the data we need in {flagging} table. I'll take look at it later tonight or during this weekend if it's not that urgent. I was trying to achieve that in https://github.com/socketwench/flag-drupal8/pull/127/files until we moved to d.o in #2410011: add a views_data handler and changed the scope of the issue.

This will be more less like DER base field views integration #2548395: Provide views relationships for DER base fields. We even have core ready for this #2378729: JoinPluginBase doesn't allow extra conditions on left table. ;-)

Edit: deleted. This patch is too complex. A simpler one is posted below.

+++ b/src/Entity/Flagging.php
@@ -129,6 +129,11 @@ class Flagging extends ContentEntityBase implements FlaggingInterface {
+    $fields['entity_id_int'] = BaseFieldDefinition::create('integer')

Really don't want this. Either we have the join as it is, or we convert the field to int (and then don't allow config entities to be flagged). I prefer the latter option.

Edit: deleted.

Edit: deleted.

But if you really dislike this solution I can regress to what comment does and just join a string to an int.

> Determines if an entity type is using an integer-based ID definition.

Arrrgh!

> But if you really dislike this solution I can regress to what comment does and just join a string to an int.

If it's good enough for core! And comment will encounter scalability issues long before flag does. So hopefully code can clean up its mess in due course.

EDIT: errrrm this comment just looks really weird now :/

Edit: deleted.

Edit: deleted.

This is the minimal patch. It's enough for now.

Actually, this is better.

+++ b/src/FlaggingViewsData.php
@@ -53,6 +54,31 @@ class FlaggingViewsData extends EntityViewsData {
+          'id' => 'standard',

We can add a reverse relationship as well.

That is already done in flag_views_data_alter . I found a bug, however: extra needs to be flag id and not entity type.

UUuuuuuuh needs tests. Yes. It does :P If someone gets to this, don't forget to assign yourself. I will do when I get to writing a test for this.

+++ b/src/FlaggingViewsData.php
@@ -53,6 +54,29 @@ class FlaggingViewsData extends EntityViewsData {
+          'help' => $this->t('The @entity_type this flagging belongs to..', ['@entity_type' => $entity_type->getLabel()]),

Double period here.

WIP patch.

Added flagging view with content relationship and test.

Removed the extra period.

Fixed a couple of trivial nits.

Mistake in the title surely?

1. +++ b/src/FlaggingViewsData.php
   @@ -48,6 +49,29 @@ class FlaggingViewsData extends EntityViewsData {
   +    foreach (Flag::loadMultiple(NULL) as $flag_id => $flag) {
   

   No need to specify NULL here.

2. +++ b/src/FlaggingViewsData.php
   @@ -48,6 +49,29 @@ class FlaggingViewsData extends EntityViewsData {
   +        $data['flagging']['flag_' . $flag_id]['relationship'] = [
   

   This is sound, but it is more limiting than providing one relationship per flaggable entity type.

   I can imagine a site builder making a list of flaggings, and having several node flags, and wanting to include node data for both those flags.

   With one relationship per flag, the node fields are then separate per flag. They'd need to faff with theming or field rewrites to collate them.

For 2., while we have "like" flags and "subscription" flags you are right that filtering on the flag_id can make that happen but the other way around it can't so yes it needs to be per entity type.

For 1., yes you need to specify NULL here, check the doxygen:

An array of entity IDs, or NULL to load all entities.

> An array of entity IDs, or NULL to load all entities.

Yes, but IIRC the NULL is the default value for the optional parameter.

Ah, you are right, of course. public static function loadMultiple(array $ids = NULL);.

#31 apply with one warning

patching file src/FlaggingViewsData.php
Hunk #2 succeeded at 76 (offset 27 lines).
patching file src/Tests/FlaggingViewTest.php
patching file tests/modules/flagging_admin/config/install/views.view.flagging_admin.yml
patching file tests/modules/flagging_admin/flagging_admin.info.yml
patching file tests/modules/flagging_admin/flagging_admin.module

#31 work for me

#31 needs work, as detailed in #33.

1) The flag link does not work with the patch.
2) There are no "flag type" filtering criteria.

> 1) The flag link does not work with the patch.

Can you explain in what way? Isn't it just that the flag link field expects to be on a flaggable entity type base table and doesn't work on flagging base table? IIRC there is a bug open for that.

> 2) There are no "flag type" filtering criteria.

I wouldn't have thought that is needed. Can you explain the use case please?

1) If I add in the fields of my view the field "Link of the flag" or "Link to delete Flagging", the link does not appear.

2) I have several flag:
- favorite flag.
- report flag .
- follow flag.
I created a flag view to display the reported entity.
This view displays all flagged entities. While I wish to have only flag type report.

> 1) If I add in the fields of my view the field "Link of the flag" or "Link to delete Flagging", the link does not appear.

Right, as I say, those are incorrectly implemented as they make assumptions about the base tables they are on. That shouldn't hold up this patch.

> I created a flag view to display the reported entity.

Ok so you need to add a filter for the flag type to the view as a whole. Doesn't that exist?

1) Thank you, but the answer is too technical for me.

2) There is no filter for the flag type.

The patch does not work with the latest dev. Here is the messsage in "composer" :

- Applying patches for drupal/flag
https://www.drupal.org/files/issues/2723703_31.patch (Add relationship to flagged entities when Flagging is base table)
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2723703_31.patch

Your requirements could not be resolved to an installable set of packages.

Problem 1
- drupal/message_subscribe 1.0.0-beta4 requires drupal/flag ~4.0 -> no matching package found.
- drupal/message_subscribe 1.0.0-beta4 requires drupal/flag ~4.0 -> no matching package found.
- Installation request for drupal/message_subscribe 1.0.0-beta4 -> satisfiable by drupal/message_subscribe[1.0.0-beta4].

1) On my screenshot, users have flagged (with a personal flag) the content "my". How can I, as administrator, remove the personal flags (without resetting the flag type) ?

2) Sorry, it works as expected. I added "type flag" when using "Report content".

3) When a comment is flagged, if I click on the title of the comment it displays the parent node. How to go directly on the comment ?

> The patch does not work with the latest dev. Here is the messsage in "composer" :

I'm afraid I'm not going to support use of composer with Flag -- I have enough problems of my own getting composer to work.

> 1) On my screenshot, users have flagged (with a personal flag) the content "my". How can I, as administrator, remove the personal flags (without resetting the flag type) ?

I don't understand the question and how it relates to the screenshot, and that's more of a support request rather than work on this issue.

> 3) When a comment is flagged, if I click on the title of the comment it displays the parent node. How to go directly on the comment ?

No idea. That's to do with Views and Comment modules, not Flag.

Please don't use issues that are about getting work done on Flag into support requests, thanks!

The patch does not work if the field is an entity rendered.

TypeError: Argument 1 passed to Drupal\views\Plugin\views\field\RenderedEntity::getEntityTranslation() must implement interface Drupal\Core\Entity\EntityInterface, null given, called in /home/www.domaine.com/public_html/web/core/modules/views/src/Plugin/views/field/RenderedEntity.php on line 108 in

would you have an idea of what would create the problem ?

I created flags:

- Follow the group
- Follow the shop

And a subscriptions view.
This view lists all the entities to which the user has subscribed.

I encounter two problem with this view:
- the user does not see the type of flag (only the administrator can see).
- can not display the group name.

> Please don't use issues that are about getting work done on Flag into support requests, thanks!

My problem is with this question, because I applied the patch to get a relationship with the groups in my view.

Right, but posting screenshots of your own specific case doens't help with fixing the problem. Particularly when they're in a different language (I happen to speak French fluently, but I can't figure out what those fields in your screenshot are, because I am not familiar with the translations of the English names of the various Views fields).

If you want to help, get a dev site set up, try to reproduce the problem in the simplest way possible. Then debug and report your findings.

I applied patch #22

The problem is that it creates several identical relationship.

As in the screenshot.

Do you have a patch for adding this feature to the dev version? Thank you

Here's an attempt to address the issues mentioned in #33.

Re-added the tests from #31. And fixed a couple code style issues.

#59 patch works correctly.

This is a very useful feature. change priority to major.

Tests has 1 fail.

A) I think the new test needs converting to a functional test

If only to move the test from the

Drupal\flag\Tests

namespace into

Drupal\Tests\flag\Functional

B) phpcs --standard=Drupal src/Tests/FlaggingViewTest.php

raises a few minor problems.

changes as described in #63

I will clean up the phpcs error in the morning

In Drupal8 all the required definitions to generate a new module is in the *.info.yml
Empty *.module files are redundant.

In terms of review from me.

like other comments above .. I can also confirm that the new view in visible and has survived my attempts to break it

The modernised tests look like they contain all the assertions needed to function as a early warning system.

so I think this is ready. . and will be a welcome addition to the project.

2723703-66.patch works well.

1. +++ b/src/FlaggingViewsData.php
   @@ -75,6 +76,66 @@ class FlaggingViewsData extends EntityViewsData {
   +    $entity_type_manager = \Drupal::entityTypeManager();
   

   Isn't this available injected already?

2. +++ b/src/FlaggingViewsData.php
   @@ -75,6 +76,66 @@ class FlaggingViewsData extends EntityViewsData {
   +      if ($base = $entity_type->getDataTable() ?: $entity_type->getBaseTable()) {
   

   There is a method on EntityViewsData that gets this IIRC.

3. +++ b/src/FlaggingViewsData.php
   @@ -75,6 +76,66 @@ class FlaggingViewsData extends EntityViewsData {
   +        // Add a relationship for bundable entities for each bundle type.
   +        $bundles = $flag->getBundles();
   

   That seems.... weird.

   Also, see my comments in #33.

In the latest patch, the relationship for bundable entities for each bundle type.
The table value for type field in extra sections is hard coded to 'node_field_data', which results in error for non node entity types. It should be something like 'table' => $base . '_flagging',

Agreed, this doesn't seem ready.

#66 Also works for me. Thanks!

I've implemented the suggestions #33 and #68:
- adding the relationship per entity type, rather than per flag
- reusing the injected service
- using the \Drupal\views\EntityViewsData::getViewsTableForEntityType method
- removed the per-bundle relationship

Still in testing my side but seems to be working well so far. Have tested with a flagging view displaying multiple node type and comment type flaggings, and the fields are pulling through as expected.

@todo:
- more testing
- update the tests in the patch (haven't got round to this yet)

Fixed issue with previous patch where the relationship didn't know which entity type was correct. Removed the tests for now as they're no longer valid until updated.

#73 works great

We (Open Social) were originally using patch #31, because we will not update to the new patch, for now, I'm re-rolling patch #31 without tests, this way it will be a valid patch for D9.

73 vs 76 (formerly 31): Looks like there are a couple of different patches here and it's not clear to me what the merits of one vs. the other are, just looking at #73 and #76. Recommendations?

Edit: looks like 73... am I right?

Hi @aharown07, yes you are right, as of now patch #73 is the most updated one, I only re-rolled version #31 because I wanted to keep the same code as before, but I needed to remove the tests.

#73 works great on Drupal 9.5.10.

Patch in #73 works well for D10.2.

Needs reroll to MR, needs tests how it works

Re-rolled The Patch #73 with The MR. Please review

Still needs tests

Can't change the MR branch to 5.x so rebased to 8.x with updated tests that are passing. Needs review.

Thanks! Merged!