Entity subqueues should be compatible with the core workspace module

Workspaces need to be changed to work with entities that have string IDs, not the other way around :)

Thank you, @amateescu!
Moving the core with IS update.

Correct component.

Adjusted title.

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

I've been thinking about this problem for the past few days and I think there's no other solution than adding a new target_entity_id_string column to the workspace_association table, and adjust all the code to use it as needed.

Assuming it was NR for thoughts on approach?

But think adding a new column makes sense. Not as familiar with entityqueue but are they doing anything off that causes this?

Agree with the tags being added.

Just for posterity reference: I asked @amateescu why we are not just converting the target_entity_id field type from int to string. The reason is that Postgres can't do joins on columns with different types.

I pushed a commit to take removed entity types into account, as I got the following error when trying to run the latest wse updates:

>  [notice] Update started: wse_menu_update_10002
>  [error]  The "wse_menu_tree" entity type does not exist. 
>  [error]  Update failed: wse_menu_update_10002 

Added test coverage for the problem we're fixing, but I'm not sure whether we're still doing upgrade path tests..

Since the update hook is adding a column and unique key think test coverage probably would be good idea though. Even if just simple

does column and key exist = no
run updates
does column and key exist now = yes

While using this MR on a site, we noticed that tables without primary keys are discouraged since https://www.drupal.org/node/3264101 → , so I changed the approach and provided default values for both target_entity_id and target_entity_id_string columns, and used them in the primary key of the workspace_association table.

In practice, this means workspaces can't support entities with a 0 integer ID (e.g. users), but I don't really see any way around that, and users were not supported anyway.

Also added upgrade path test coverage, this should be fully ready for reviews now.

The solution to restore the primary key and default to 0 for the int ID makes sense to me: users are not supported and even if they were, the anonymous user wouldn't be editable. Any content entity out there with numeric ID relying on core base classes will start from 1, so I think this is a safe approach.

The MR code has been working nicely in production for a while now, except for the latest update, and tests look good, so I think we are done here.

Sorry I have one question:

target_entity_id is currently 'not null' => TRUE, however if target_entity_id_string is set, then it can and should be unset.

So... instead of adding a default value of 0, why not make target_entity_id 'not null' => FALSE?

The primary key would still enforce that either target_entity_id or target_entity_id_string has a value, or at least that more than one row has a value, because two rows with the same null/empty values would be forbidden. So from that perspective it's just as strict as the current MR, since you could have one row with 0/'' in there as it is.

Theoretically and for the same reason, target_entity_id_string could also allow null.

@catch, that's because we enforce all columns of a primary key to be 'not null' => TRUE since #2616724: Warn when trying to create a database table with a NOT NULL => FALSE primary key → .

The alternative would be to create a UNIQUE key instead, and that's what the MR did at first (essentially implementing your proposed schema definition for those columns), but the mysql db driver complains about tables with missing primary keys since 🐛 Use READ COMMITTED by default for MySQL transactions Fixed , see mysql_requirements().

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rerolled.

This issue is very tricky, both postgres not supporting joins on int vs. string columns and the primary key/not null handling, however it looks like all alternatives have been exhausted so let's go ahead.

Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!

catch → closed merge request !9568

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