Problem/Motivation

Currently, choices are stored as a field item. However, poll votes need to be assigned to a specific choice, even if the order changes. Field items have no unique ID, only the delta, and that changes when you reorder them.

Choices currently attempt to solve that by adding an auto-incremented choice ID. the schema for that was ported from the previously separate table and uses things that only work on MySQL, like the serial on a non-primary key column (it defines it, but that would result in two primary keys which can't work and is ignored).

Additional complexity is added by #132339: Multilingual poll choices, in translations, choices might be added or removed, making it impossible that the choice ID is kept in sync.

Proposed resolution

The only clean solution to this problem that we can see is to make choices a separate entity type that can be translated too. Then choices become a reference field with a special widget that allows inline editing and translation, similar to paragraphs and IEF.

As a side effect, this opens up a bunch of possibilities for later, like the ability to add configurable fields to choices to support features like images instead of tests for the choices.

The only other alternative that we can think of would be to expose the choice ID in the UI and switch to a random value/machine name/sequence, that users must keep unique and in sync between translations. That would be very bad UX.

Remaining tasks

Implement it.

User interface changes

Minimal, for now.

API changes

All choice related code needs to be updated to use the separate entity type instead.

Data model changes

A new entity type with base + data table. the choices field goes away. Migration for this is needed.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Status: Active » Needs review
FileSize
14.98 KB

Haven't run the tests yet but manual testing seems to be working surprisingly well.

TODO:
* Updating tests
* Write an update function (that's going to be fun...)

Status: Needs review » Needs work

The last submitted patch, 2: choice-entity-type-2641828-2.patch, failed testing.

Berdir’s picture

Fixed deletion, completely removed vote value, fixed and improved tests

Berdir’s picture

Ok, here's the update function, this was fun to write (hint: not really).

plach’s picture

  1. +++ b/poll.install
    @@ -65,3 +67,57 @@ function poll_schema() {
    +  $poll_choice = \Drupal::entityTypeManager()->getDefinition('poll_choice');
    

    On installation it's probably not necessary (or maybe not even possible), but as a general rule getting the entity type (or field storage) definition from the entity updates manager is preferable, as it's retrieved from state instead of code. This makes the update more reliable.

  2. +++ b/poll.install
    @@ -65,3 +67,57 @@ function poll_schema() {
    +  // @todo: There has to be a better way to do this.
    

    Doesn't regenerating the schema from scratch work? That is, moving out data from the schema, performing a "blank" update (as in the second example at https://www.drupal.org/node/2554097) to drop and re-create the schema, and moving the data back into the new schema?

  3. +++ b/src/Plugin/Field/FieldWidget/PollChoiceDefaultWidget.php
    @@ -35,25 +36,102 @@ class PollChoiceDefaultWidget extends WidgetBase {
    +      // If target translation is not yet available, populate it with data from the original paragraph.
    ...
    +      // Initiate the paragraph with the correct translation.
    

    I somehow suspect you got inspired by existing code :)

    It would be great to have a way to factor it to some common API sooner or later...

  4. +++ b/src/Tests/PollBlockTest.php
    @@ -46,6 +46,7 @@ class PollBlockTest extends PollTestBase {
    +    debug($options);
    

    leftover

Berdir’s picture

Thanks!

1. Yes, it's not possible on install. In fact, we have a bug in system_update_8004(), if you have a *new* entity type in the system while running that, it fails horribly. There's an unrelated issue that has a fix (rest config refactoring) haven't opened a separate issue yet.

2. Hm. I'm not sure I like the fetch all, delete then resave approach, for two reasons. 1. It seems somewhat risky, if anything goes wrong during the update, then your data is gone? 2 It doesn't scale. My code would be relatively easy to convert into a batch process when you have a lot of data, that's not the case for the example in the change record.

3. Ha, yes. I think @swentel opened an issue to move an API or something for inline entity form in core. I want to extend this to actually use a form display and show all fields inline, that would allow for use cases like pictures in poll choices. But this step is big enough on its own.

Berdir’s picture

Status: Needs review » Fixed

Fixed 3. and 4, committed this to make progress. Thanks again for the review.

  • Berdir committed 3727c98 on 8.x-1.x
    Issue #2641828 by Berdir: Convert choices to a separate entity type
    
plach’s picture

Hm. I'm not sure I like the fetch all, delete then resave approach, for two reasons. 1. It seems somewhat risky, if anything goes wrong during the update, then your data is gone? 2 It doesn't scale. My code would be relatively easy to convert into a batch process when you have a lot of data, that's not the case for the example in the change record.

Well, that example was for a test entity type, I think I mentioned somewhere it has scaling issues, and that a temporary storage should be used in real-life use cases. Probably also the CR should mention that. With a temporary storage a batch process would be viable also in that case, and would address your concerns.

Anyway, the API was specifically designed not to cope with this kind of updates requiring a migration, and that's the reason why it does not expose the logic to retrieve a Schema API definition. I was assuming Migrate would be the solution in this cases, but obviously an update function is more handy, if viable. De-encapsulating that logic would be easy although it may allow people to circumvent the Entity Storage API.

In general my answer would still be: use the "drop/re-create + temp storage" approach, but if it turns out it's not practical, well, extending the API is certainly a solution.

Status: Fixed » Closed (fixed)

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