Problem/Motivation

  • target_id_int is kept synchronized with target_id by a database trigger. Per #3204103-12: Use virtual index instead of triggers, this is useful in case any code writes to the DER field's database tables directly, without going through Drupal's Entity API.
  • However, per #2930423: MySQL triggers aren't supported on all hosting platforms, database triggers aren't supported on all hosting platforms.
  • Futhermore, writing to Drupal's entity or field tables without going through Drupal's Entity API is problematic for plenty of other reasons, such as then various hooks don't get called. Therefore, many sites don't do this.

Steps to reproduce

Proposed resolution

Change DynamicEntityReferenceItem to also keep target_id_int in sync via Drupal. I think that all that needs to be done for this is:

  • DynamicEntityReferenceItem::schema() would need to add target_id_int as another column.
  • DynamicEntityReferenceItem would need to override __get() such that when target_id_int is passed, it returns the integer value of target_id, otherwise call the parent getter.

This could be in addition to leaving the existing trigger approach in place, so that sites where triggers are supported continue to get the benefit of the column staying synchronized even when target_id is updated in the database via something other than the Entity API. But this would enable sites where triggers aren't supported to also be able to use DER 4.x, so long as those sites don't update DER's database values without using the Entity API.

Remaining tasks

  • The latest patch has duplicate code creating the int column. It's now part of the field item schema, but the IntColumnHandler classes still create it too. So we need to remove the column creation from the IntColumnHandler classes while leaving the trigger creation in place.
  • The latest patch still needs a hook_update_N() function written in order to make the update path tests pass.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#12 der-3276723-12.patch13.77 KBeffulgentsia
#11 der-3276723-11.patch13.7 KBeffulgentsia
#8 der-3276723-8.patch7.02 KBeffulgentsia
#7 der-3276723-7.patch6.78 KBeffulgentsia
#5 der-3276723-5.patch764 byteseffulgentsia
#4 der-3276723-4.patch1009 byteseffulgentsia

Issue fork dynamic_entity_reference-3276723

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

effulgentsia created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

This sounds a hell of a lot simpler
I'd be in favour of it

effulgentsia’s picture

effulgentsia commented
Title: Use Drupal instead of a trigger to keep target_id_int synchronized » In case database triggers aren't supported, update target_id_int within Drupal as well
Issue summary: View changes

Updating title and summary based on #3204103-12: Use virtual index instead of triggers.

effulgentsia’s picture

effulgentsia commented
Status: Active » Needs review
StatusFileSize
new der-3276723-4.patch1009 bytes

First, just wanting to see which tests fail without the trigger. NR for testbot only.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new der-3276723-5.patch764 bytes

Coding standards checks aren't happy with commenting out lines of code, so removing them instead.

Status: Needs review » Needs work

The last submitted patch, 5: der-3276723-5.patch, failed testing. View results

effulgentsia’s picture

effulgentsia commented
Status: Needs work » Needs review
StatusFileSize
new der-3276723-7.patch6.78 KB

Here's a start. Still needs work, but NR for testbot.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new der-3276723-8.patch7.02 KB

Status: Needs review » Needs work

The last submitted patch, 8: der-3276723-8.patch, failed testing. View results

effulgentsia’s picture

effulgentsia commented
Assigned: Unassigned » effulgentsia

I'm pausing work on this for now, but assigning to myself to remember to pick it up again soon.

effulgentsia’s picture

effulgentsia commented
Assigned: effulgentsia » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new der-3276723-11.patch13.7 KB

This adds test subclasses for each test that failed in #5. This way, the pre-existing tests keep testing with the pre-existing IntColumnHandler classes, and these new tests run those same tests again with a NULL implementation of IntColumnHandler to emulate an environment where database triggers don't run.

The kernel tests all pass, but the update path tests all fail, because this patch still needs to implement a hook_update_N() function. That might be a bit tricky. Not sure when I'll have time to get to it, so unassigning myself for now.

In addition to the update function, still todo on this patch is to also change the pre-existing IntColumnHandler classes to not create the column (since the field item now does that), and to limit themselves to only creating the trigger.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new der-3276723-12.patch13.77 KB

Fixed coding standards violations.

Status: Needs review » Needs work

The last submitted patch, 12: der-3276723-12.patch, failed testing. View results

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented 
+++ b/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
@@ -481,6 +488,54 @@ class DynamicEntityReferenceItem extends EntityReferenceItem {
+    else {
...
+    else {
...
+    else {
...
+    else {

nit, no need for else here

(die else die 🏴‍☠️)

In terms of the update, we can look at how date_recur added a new column https://cgit.drupalcode.org/date_recur/commit/date_recur.install?id=8039... for an example

ronraney’s picture

ronraney commented

Has there been a resolution or change to this since 2022? I'm not certain, but we may have an issue with triggers as it relates to our hosting platform. I see a "failed testing" for most or all of the patches.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

I'd say #3204103: Use virtual index instead of triggers is the way forward here. I'm planning to work on it in a couple of weeks time.

ronraney’s picture

ronraney commented

Hello, I'm running out of time due to changes in MySQL. We've had a workaround in place that is no longer working. It the only working solution to downgrade to 1.x(3.x in D10)?

larowlan opened merge request !57

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 4.x-dev » 5.x-dev
Status: Needs work » Needs review

  • larowlan committed e4532d26 on 5.x 
    feat: #3276723 In case database triggers aren't supported, update...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Fixed

Committed to 5.x dev

Looking for folks to test the update path and report back

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.