Allow config entities to be flagged

For example because flag can't store non-numeric entity ID's. So you can add flags, but it will fail to actually flag entities.

Flag 8 did support flagging config entities at one time. We never removed the ability within the fields, only added a block in EntityFlagTypeDeriver.

I believe it was Joachim that suggested we remove support for config entities.

A first problem is that Dynamic Entity Reference appears to only allow referencing content entities, and that's something we might be moving towards using (in fact, chx suggested it on an issue recently I think).

Upping this to major since I want this resolved before beta releases.

> But now we have all the code not to need DER: we have an entity reference field on our own and using DER would only allow you to rip out the 15 or so lines of code I added recently and not much more.

Would DER not provide some of the Views support for us?

An entity_reference stores just ID's, for a single type. And the schema that it generates is for a single entity type. What comment.module does is a) cheating and b) doesn't support config entities either.

DER offers an entity_type/entity_id reference as a generic pattern. However, it currently actually only supports content entities too, so it wouldn't help us here.

Coming from #2555027-13: Support non-numeric entity ID's quoting myself here.

I had a chat with @larowlan today and we both think that it'd be good to add a new field type for sting entity ID's so that we don't end up in the mixed situation. ER is doing the same thing as well.

Other then flag #2678756: Allow config entities to be flagged I don't see any use case of mixed entity IDs. flag can have two DER basefields one for int entity id and other is for sting entity id and flagging/unflagging action or preSave can populate the respective DER field base on flagged entity id.

This seems viable given that we might end up doing it in #2723703: Add relationship to flagged entities when Flagging is base table anyway.

Re: @joachim from #2657384-10: Flagging should use some standard entity reference

dynamic_entity_reference looks like it might be viable at some point in the future. My concern with it previously was the support for Views, but that looks like it might be ok now. Though we probably don't want to be adding a dependency on a module that's not yet stable, so waiting till it lands in core seems best.

As per DER status report the module is stable for quiet a while now. There are only two pending issues left in #2447963: [meta] DER roadmap for 8.x-1.x. I'm planning to fix them by the end of this month so that DER can have a stable release.
As far as moving it to core is concerned #2407587: Allow multiple target entity types in the entity reference field we need a product manager approval afaik no one is working on that.

imo #2555027: Support non-numeric entity ID's is a new feature and it can get in anytime we want without any disruption to stable release. The follow up of #2555027 would be to add views integration. Given that DER already support int entity id that would not be a huge task.

Still needs tests against config entities

This is a rebase of #15, I'm going to work on some tests.

The idea of flagging config entities is of use over here in message_subscribe as a way to subscribe to, for instance, all content of a given type.

Here's a very basic test. It caught a needed schema change to flag_counts.

One question I had: should this behavior be configurable, defaulting to false, so the UI isn't cluttered with config entities for the simple use cases?

The above patch contains similar changes to #2546074: Update flag counts table schema, but that issue will need to be committed before this one, and then those duplicate changes removed here.

he whole issue is a gigantic mess there's no way out of this but DER 8.x-2.x

So what's the call, then? The proper solution sounds like using DER, but the most recent patch doesn't use it.

It seems like if changing to a string isn't enough, then this issue should require DER and go from there?

> DER 8.x-2.x

DER (https://www.drupal.org/project/dynamic_entity_reference) has a RC on the 1.x branch; the 2.x branch is in dev.

Needs a summary update to explain the situation clearly.

DER 2.x branch is in alpha state for quite a while now but I don't want to make a release until #2766175: Fix the Views integration for entities with string ids. and #2808337: Make use of proposed entity table relationship refactor are fixed. And once #2766187: Allow config entity references is fixed we'll release the beta.

Moving to postponed given #26.

Removing the postponed status, as the remaining issues for DER need not block this. I'll start working on a patch that utilizes DER.

This will fail on the testbot, because new module dependencies are not processed for patches. All tests are green locally. Posting this sooner rather than later to get some feedback.

Still at NR.

I'm really glad to see the practical use of DER.

• +++ b/flag.info.yml
  @@ -4,3 +4,5 @@ core: 8.x
  +  - dynamic_entity_reference
  

  This should be dynamic_entity_reference:dynamic_entity_reference (2.x). As per https://www.drupal.org/node/2000204 and https://www.drupal.org/node/542202#dependencies

• +++ b/src/FlagService.php
  @@ -140,8 +140,8 @@ class FlagService implements FlagServiceInterface {
  +        ->condition('flagged_entity__target_id', $entity->id());
  
  @@ -159,8 +159,8 @@ class FlagService implements FlagServiceInterface {
  +      ->condition('flagged_entity__target_id', $entity->id());
  
  @@ -187,8 +187,8 @@ class FlagService implements FlagServiceInterface {
  +      ->condition('flagged_entity__target_id', $entity->id());
  
  @@ -236,8 +236,10 @@ class FlagService implements FlagServiceInterface {
  +        'target_id' => $entity->id(),
  
  @@ -294,8 +296,8 @@ class FlagService implements FlagServiceInterface {
  +      ->condition('flagged_entity__target_id', $entity->id());
  
  +++ b/src/Tests/AdminUITest.php
  @@ -197,8 +197,8 @@ class AdminUITest extends FlagTestBase {
  +      ->condition('flagged_entity__target_id', $this->node->id())
  

  Just make sure the condition is between same type of columns. There is \Drupal\dynamic_entity_reference\Plugin\Field\FieldType\DynamicEntityReferenceItem::entityHasIntegerId() function in #2766175: Fix the Views integration for entities with string ids. you can use.

This sets the proper version for DER. I'll fix the remainder of #34 once #2766175: Fix the Views integration for entities with string ids. has been committed, since it has the method needed to check int vs string.

Hmm, that change might not work until there is an official release of DER?

Note message_subscribe depends on flag:

Module message_subscribe cannot be enabled because it depends on dynamic_entity_reference  (2.x) but  is available

I moved \Drupal\dynamic_entity_reference\Plugin\Field\FieldType\DynamicEntityReferenceItem::entityHasIntegerId to 8.x-2.x in https://www.drupal.org/commitlog/commit/65971/de0b1d85476037ed669bea835a...

This attempts to take into account the switching target id column. However, note the @todos. I could not get the _int column to work with EFQ at all. Is that a known issue?

This is the exception:

Drupal\Core\Entity\Query\QueryException: 'flagged_entity__target_id_int' not found

I also rolled this patch utilizing the new method added over in #2827219: Add helper method to retrieve complete column name, but can reroll if it is determined that helper method won't go in.

I could not get the _int column to work with EFQ at all. Is that a known issue?

#2808337: Make use of proposed entity table relationship refactor will fix that.
I'll have a look at #2827219: Add helper method to retrieve complete column name later today.

Re-roll to utilize the latest helper method in #2827219: Add helper method to retrieve complete column name. Note that #2808337: Make use of proposed entity table relationship refactor, which is test-only, has not resolved the issue in #38. When the query builder looks for column mappings in Tables::ensureEntityTable(), the _int column is not listed as one of the table mappings:

  protected function ensureEntityTable($index_prefix, $property, $type, $langcode, $base_table, $id_field, $entity_tables) {
    foreach ($entity_tables as $table => $mapping) {
      if (isset($mapping[$property])) {
        if (!isset($this->entityTables[$index_prefix . $table])) {
          $this->entityTables[$index_prefix . $table] = $this->addJoin($type, $table, "%alias.$id_field = $base_table.$id_field", $langcode);
        }
        return $this->entityTables[$index_prefix . $table];
      }
    }
    throw new QueryException("'$property' not found");

as such, the @todos are still in this patch.

One question, shouldn't DER, in the case of an EFQ, take care of the string/int column logic already? In which case these @todos could simply be removed...

This is postponed on #2808337: Make use of proposed entity table relationship refactor. In #40 I hadn't realized there was more in that issue than just the test.

Let's fix #2808337: Make use of proposed entity table relationship refactor and after that we can create an alpha tag for DER.

This would require handling of the 'show as field' and 'show on form' options.

Also, could someone explain the use case? I don't understand how the screenshot is showing a use of Flag.

I created 8.x-2.0-alpha1.

Also, could someone explain the use case? I don't understand how the screenshot is showing a use of Flag.

I'm approaching this issue from a very similar spot to that screenshot. In the message_subscribe module, Flag is used for users to subscribe or unsubscribe from things. For subscribing to comments on a node, the node entity gets flagged. For subscribing to all content of a given term or user, the term or user is flagged.

The big missing piece there is subscribing to all content of a *content type*. The most elegant solution to that would be to flag the particular content type.

I've updated the IS accordingly.

Here's a reroll now that #2801423: FlaggingStorage does not update cached Flagging status. and #2808337: Make use of proposed entity table relationship refactor are in. The @todos are removed because DER now determines which field to use for EFQs.

Due to the way the drupal testbots work, adding a new module dependency via a patch does not download that for the tests. Only once the .info.yml file is committed are new dependencies registered, so until then, these tests won't pass on the testbot. They are all green locally.

Unavailable module: 'dynamic_entity_reference'

Here's a reroll against the latest 8.x-4.x. As mentioned above, tests for this will not pass on the test bot until DER is committed as a dependency, but I have re-verified this is passing locally for all tests.

We can create a feature branch now to run the tests on d.o.

We can create a feature branch now to run the tests on d.o.

Great idea! Let's do that.

+++ b/src/Entity/Flagging.php
@@ -162,10 +155,10 @@ class Flagging extends ContentEntityBase implements FlaggingInterface {
-    if ($name == 'entity_id' && $this->get('flagged_entity')->isEmpty()) {
+    if ($name == 'flagged_entity' && $this->get('flagged_entity')->isEmpty()) {
       $this->flagged_entity->target_id = $this->entity_id->value;
     }

AFAICT this used to keep the computed field up to date. What does it do now? Also, 'entity_id' is no longer a base field, is it?

re #54 hmm, excellent question. I'm guessing it can be removed since DER would take care of computing/gathering the entity?

I am happy with the idea behind this issue.

I have a kink to report, which is only visible if you first have NOT downloaded dynamic_entity_reference

It looks like the dependencies are not setup correctly,

here is the output from "drush en flag" with the patch applied.

drush en flag
The following projects have unmet dependencies:                                               [ok]
flag requires dynamic_entity_reference
Would you like to download them? (y/n): y
Project dynamic_entity_reference (8.x-1.1) downloaded to                                      [success]
XXXX/drupal//modules/dynamic_entity_reference.
Module flag cannot be enabled because it depends on dynamic_entity_reference  (2.x) but 1.1 is[error]
available

We won't be able to address #56 here--that's an issue with Drush preferring stable releases/and also not parsing the version requirements out of .info.yml files. The error output there should be sufficient to guide people towards the correct version of DER though.

The matter of how people with existing sites are going to update their data came up at #2845567: make use of ContentEntityBase::baseFieldDefinitions(); make flag_id an entity reference field...

I'm minded to say if this issue gets in 4.x (as opposed to future 5.x), so does that one, as two painful field changes isn't that much worse than one...

This should address the onChange method. We also no longer needed the constructor method, so that is removed.

This is at NR since tests won't pass on the testbot until we commit (or commit to a feature branch).

...or commit to a feature branch.

Can we try that?

Yes, you can. Only, the maintainers can do that. You have to create a branch 2678756-allow-config-entities and commit this patch and push it to d.o.

*Finally* got around to committing that to a feature branch. I hope I did it right?

Yeah, it is correct I think there is a bug in composer workflow on testbot.

02:30:27 Problem 1
02:30:27 - The requested package drupal/flag dev-2678756-allow-config-entities exists as drupal/flag[dev-4.x, 4.x-dev, 4.0.0-alpha1] but these are rejected by your constraint.

@jibran where did you see the test results for this branch?

https://www.drupal.org/pift-ci-job/592232

I spoke with @Mixologic, and these are known failures (feature branches cannot currently be tested). I've rerun all these tests locally and they are still green.

Also, even though Flag is in alpha, I've added an update hook:

/**
 * Update flagging table to use dynamic_entity_reference.
 */
function flag_update_8001() {
  if (!\Drupal::moduleHandler()->moduleExists('dynamic_entity_reference')) {
    try {
      /** @var \Drupal\Core\Extension\ModuleInstallerInterface $installer */
      $installer = \Drupal::service('module_installer');
      $installer->install(['dynamic_entity_reference']);
    }
    catch (MissingDependencyException $e) {
      throw new UpdateException('The dynamic_entity_reference module could not be found. Please download and rerun database updates.', $e->getCode(), $e);
    }
  }
  
  // Previous schema was `entity_type` and `entity_id`. New schema is
  // `flagged_entity__target_type` and `flagged_entity__target_id`.
  // @see \Drupal\dynamic_entity_reference\Plugin\Field\FieldType\DynamicEntityReferenceItem::schema()
  $spec = [
    'description' => 'The Entity Type ID of the target entity.',
    'type' => 'varchar_ascii',
    'length' => EntityTypeInterface::BUNDLE_MAX_LENGTH,
  ];
  Database::getConnection()->schema()->changeField('flagging', 'entity_type', 'flagged_entity__target_type', $spec);

  $spec = [
    'description' => 'The ID of the target entity.',
    'type' => 'varchar_ascii',
    'length' => 255,
  ];
  $keys = [
    'target_id' => ['flagged_entity__target_id', 'flagged_entity__target_type'],
  ];
  Database::getConnection()->schema()->changeField('flagging', 'entity_id', 'flagged_entity__target_id', $spec, $keys);

  /** @var \Drupal\dynamic_entity_reference\Storage\IntColumnHandlerInterface $int_column_handler */
  $int_column_handler = \Drupal::service('dynamic_entity_reference.storage.create_column');
  // Adds the integer column.
  $int_column_handler->create('flagging', ['flagged_entity__target_id']);

  // Populate the int column.
  Database::getConnection()->update('flagging')
    ->expression('flagged_entity__target_id_int', 'flagged_entity__target_id')
    ->execute();
}

we could add this to the patch, or just direct folks to this comment who need the update hook.

And the same update hook in patch form.

Any thoughts on how to move forward with this?

@jhedstrom let's create a PR on github so that we can run tests.

Maybe this will work.

Small bug in the update code:

  // Adds the integer column.
  $int_column_handler->create('flagging', ['flagged_entity__target_id']);

That's not the integer column that the code is adding.

And this is probably a problem in DER rather than here, but EntityTypeInterface::BUNDLE_MAX_LENGTH is for bundle names, not entity type names.

And this is probably a problem in DER rather than here, but EntityTypeInterface::BUNDLE_MAX_LENGTH is for bundle names, not entity type names.

Please have a look at http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Field/Plugin....

That's not the integer column that the code is adding.

flagged_entity__target_id is a string column and the new cloumn would be flagged_entity__target_id_int so I don't think there is anything wrong with the update code.

Do we want to add update path as well? If yes then let's do that in follow-up with proper tests.

1. +++ b/flag.install
   @@ -103,3 +107,49 @@ function flag_requirements($phase) {
   +  Database::getConnection()->schema()->changeField('flagging', 'entity_type', 'flagged_entity__target_type', $spec);
   ...
   +  Database::getConnection()->schema()->changeField('flagging', 'entity_id', 'flagged_entity__target_id', $spec, $keys);
   

   Let's create a local variable.

2. +++ b/flag.install
   @@ -103,3 +107,49 @@ function flag_requirements($phase) {
   +  $keys = [
   

   I think this should be $keys['indexes'].

3. +++ b/flag.install
   @@ -103,3 +107,49 @@ function flag_requirements($phase) {
   +  // Populate the int column.
   

   I'd like this to be in a separate update hook but DER is doing the same thing so I think this is correct because create_column service is doing the proper exception handling.

4. +++ b/flag.install
   @@ -103,3 +107,49 @@ function flag_requirements($phase) {
   +    ->expression('flagged_entity__target_id_int', 'flagged_entity__target_id')
   

   DER is doing update($table)->fields([$int_column => 0])->execute()instead

5. I think update hook is also missing the key-value collection update for tracking installed storage schema. i.e. \Drupal::keyValue('entity.storage_schema.sql'); should also be updated see der.install:99 for more details.

@jhedstrom do you think, we can do something similar to #69 for #2823528: Allow conversion of ER fields to DER fields as well?

Agreed we should add the update hook in a follow-up (or not at all since we haven't been previously doing update hooks while in alpha).

@jibran I think the approach in #69 could be done in a more generalized manner to address #2823528: Allow conversion of ER fields to DER fields. It definitely works as a one-off as I have manually tested it on a site with flagging data.

> flagged_entity__target_id is a string column and the new cloumn would be flagged_entity__target_id_int so I don't think there is anything wrong with the update code.

Yes, but the code and the comment don't agree!

  // Adds the integer column.
  $int_column_handler->create('flagging', ['flagged_entity__target_id']);

Comment says 'integer column', but the code is adding the string column.

> Do we want to add update path as well? If yes then let's do that in follow-up with proper tests.

Nope, we're still not at stable release yet. I've said before we should collect update code on a single support issue, so that people who are using Flag in production (despite the warnings on the project page!) can use it & contribute.

It's a bit confusing, but that method is creating the int column. Its argument is the non-int target_id column name. From IntColumnHandlerInterface:

  /**
   * Creates the _int columns and the triggers for them.
   *
   * @param string $table
   *   The non-prefix table to operate on.
   * @param array $columns
   *   The DER target_id columns.
   *
   * @return array
   *   The list of new target_id_int columns.
   */
  public function create($table, array $columns);

I've said before we should collect update code on a single support issue, so that people who are using Flag in production (despite the warnings on the project page!) can use it & contribute.

This is a fantastic idea!

I've added #2858553: Pre-beta update hook code. What else needs to be done here?

Nothing if you are asking me.

... just that DER needs a stable release.

Also, would be good if the issue summary could mention the resolution to use DER, with a link to the project.

As I said in #26 it is almost in beta state. I can release RC and as soon as the current queue is cleared I'll release the stable version. After RC there would be no API change and DER will support the upgrade path.

Would you agree on using DERv2 rc for flag alpha and beta release phase?

Given how far Flag is from RC itself, and that DER seems to be advancing faster than us, yup, RC is fine.

It seems this need not wait for a DER RC since Flag is still in alpha? Should something big change in DER, we could keep this code base in sync as needed...

Added the requested info to the IS.

Shouldn't this still be Needs Work for #77.2?

@jonathanshaw that was feedback regarding the upgrade path, which isn't part of this patch, but is over in #2858553: Pre-beta update hook code now.

I'd like to see this committed so that various contrib modules requiring flag can start to rely on this functionality... as mentioned in #87, since both DER and Flag are still in alpha, if something in DER changes, that change can quickly be incorporated here too...

Reminder to self, this patch is waiting on DER 2.x to get to the RC stage.

https://www.drupal.org/project/dynamic_entity_reference/releases

Let's fix #2867102: Incompatibility with jsonapi and come up with the plan for #2835542: Utilize service decorator instead of taking over entire EFQ service in a way that we can make the changes without BC break(or won't fix the issue) then I'm happy to release DER RC.

I have faced with the error from issue https://www.drupal.org/project/flag/issues/2864440
And patch #75 helped me.
But there is Flag 4.0.0-alpha3 version module on the project so I recreated patch for this version.
Maybe it will useful for somebody.

Minor cleanup from that re-roll

This is just a trivial correction -- but in other news it looks like our random test failure is raising its ugly head.

The general trend in core is to migrate away from array() to [].

I am just reversing a movement in the other sense - and maintaining the use of [].

I've been ignoring this because I'm not sure what to do with it. It's definitely not a beta blocker, because beta is out and this adds a lot of complexity.

At least it will need an upgrade path, which is going to be a challenge.

I have made a commit to the issue fork in branch 8.x-4.x, which applies patch #98 2678756-97.patch by xdong and patch #70 2678756-update-hook-do-not-test.patch by jhedstrom, implementing support for flagging config entities. I have tested this on a local WAMP installation of Drupal 9.4.2 running on PHP 8.0.13-nts via fastCGI, and everything seems to be working as expected. I can flag both content and config entities as normal, and all the data stored in the flagging and flag_counts tables looks good.

A merge request has been created for my commit, maintainers would you be able to have a look at this and review it?

Can we have this functionality looked at again?

We are in the process of migrating from Drupal 7 to Drupal 9 and have a Flag attached to a bundle so users can subscribe to notifications for new content.

In Drupal 9, this entity is now a config entity which means we can't migrate the Flag unless support for this is added.

Re-roll of the patch from #98 for 4.0-beta4.

Oops, minor goof in the re-roll.

And it looks like some additional tweaks are necessary if you're using DER 3.x.

Looks like I missed something for the DER 3.x version.

Needs rebase

Just to add a little clarification: phpunit run with MySql fail, because of the table prefix which is test, this is then considered as a system table which then triggers the error. Updated the gitlab-ci so it runs mysql and pgsql in parallel as pgsql does not have the test issue/limitation. As seen from that, tests are failing so this is not ready to be merged and needs some additional work.

The tests that are failing for pgsql are in relation to the views and the relationship comparison "operator does not exist: character varying = bigint" and "invalid input syntax for type bigint". There is a patch for core that should solve this issue so not something that needs to be handled in Flag: #3079534: Views JOIN condition fails in PostgreSQL when comparing entity identifiers of different datatypes. And there is also an issue opened in flag: #2864440: PostgreSQL problems on entity_id of type varchar with flagging and flag_count tables in regards to that.

With MySql DB locally all the tests are passing. On Drupal CI the issue with MySql is with the naming of the DB tables with the test prefix, which then makes them a system table and triggers can't be triggered on system tables. Tried to rename the tables with no luck.

Leaving the status to needs work just because of the tests issues, but would be good if somebody could test manually and confirm if everything works as expected. And then maintainers can decide on how to proceed.

The system table thing isn't the table name but the database, see #3463044: Do not use the system "mysql" database to install Drupal.

This needs to be corrected in the gitlab-ci templates as when changing the db the db is then not found
PHP Fatal error: Uncaught PDOException: SQLSTATE[HY000] [1049] Unknown database 'drupalci_db' in /builds/issue/flag-2678756/web/core/modules/mysql/src/Driver/Database/mysql/Connection.php:142. Not sure if creating the DB would help solve the issue.
Will revert the changes made to gitlab-ci.yml file.
The changes can be tested manually, this is now just an issue with the gitlab-ci testing.