Problem/Motivation

For legacy reasons we enable bulk form on base and field data entity tables. This is very confusing from the UX perspective as they both appear in the UI.

Proposed resolution

Remove the one that is added to the base table since the other table is used by default. Provide update hook for existing installations that are using removed field.

Comments

slashrsm created an issue. See original summary.

slashrsm’s picture

Status: Active » Needs review
StatusFileSize
new964 bytes

This still needs an update hook.

samuel.mortenson’s picture

I'll work on the update hook.

samuel.mortenson’s picture

StatusFileSize
new2.28 KB
new2.86 KB

OK, I added an update hook for this and also changed the logic in entity_browser_views_data_alter() to check for the data table instead of the base table before adding the "entity_browser_select" field.

Status: Needs review » Needs work

The last submitted patch, 4: 2770137_4.patch, failed testing.

The last submitted patch, 4: 2770137_4.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
StatusFileSize
new2.9 KB
new684 bytes

Added a check to prevent the notices from the last test run.

Status: Needs review » Needs work

The last submitted patch, 7: 2770137_7.patch, failed testing.

The last submitted patch, 7: 2770137_7.patch, failed testing.

samuel.mortenson’s picture

Ah - this is going to be hard or impossible to implement as some Entities, like File, have no Data Table:

>>> \Drupal::entityTypeManager()->getDefinitions()['file']->getBaseTable()
=> "file_managed"
>>> \Drupal::entityTypeManager()->getDefinitions()['file']->getDataTable()
=> null

Does the patch need to be re-written to remove the duplicate using the data table instead of the duplicate using the base table?

slashrsm’s picture

I guess. I checked how core adds bulk form to entities and it seems that it is always added to the base table. Safe to assume that we can do the same?

samuel.mortenson’s picture

OK - I'll work on a re-roll to remove/migrate the data table field, and look into test coverage.

samuel.mortenson’s picture

Status: Needs work » Needs review
StatusFileSize
new9.78 KB
new11.44 KB

Here's a re-roll with a test (which I can't get running locally, strangely enough).

Status: Needs review » Needs work

The last submitted patch, 13: 2770137_13.patch, failed testing.

The last submitted patch, 13: 2770137_13.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
StatusFileSize
new639 bytes
new11.45 KB

file_get_contents was using a relative dir, should be fixed.

Status: Needs review » Needs work

The last submitted patch, 16: 2770137_16.patch, failed testing.

The last submitted patch, 16: 2770137_16.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
StatusFileSize
new1.15 KB
new11.83 KB

Hopefully fixed some minor bugs in the tests.

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community

Looks OK to me. Also tested it manually and can confirm that the view is updated.

slashrsm’s picture

Also asked @berdir to look at this. He mentioned that it might be more performant to use data table for entities that have them (to avoid unnecessary join). However, we tested this and it turns out that the base table is never joined as a result of the bluk form field.

Should be good to go.

slashrsm credited Berdir.

slashrsm credited Berdir.

slashrsm’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks!

Status: Fixed » Closed (fixed)

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