The Vote entity extends ContentEntityBase, but ignores and re-implements many features already provided and implemented in ContentEntityBase. I alluded to this in #3150371: Vote/VoteInterface are documented improperly and incompletely

For example, ContentEntityBase defines base fields id, uuid, language, etc. which are common to ALL entities. Because Vote extends ContentEntityBase, Vote does not need to redefine these base fields, it just needs to properly delegate to the parent class. Because Vote is currently ignoring the parent class, we are duplicating code and losing functionality. Plus this duplicate code makes it harder to maintain - if a bug is fixed or a new feature added to the parent ContentEntityBase, that bug fix / new feature will NOT currently be inherited by Vote.

Here is a work in progress. The point of this patch is to invoke parent::baseFieldDefinitions() within Vote::baseFieldDefinitions so that the Vote entity will use the parent's implementation of the base fields. Right now Vote has a copy of those definitions from the parent rather than just inheriting them.

Note that this patch does not change any existing functionality of the Vote entity, but after the patch the Vote entity will have a few new base fields inherited from ContentEntityBase, so there will need to be a hook_update_N() to update the Vote entity on existing sites. That's why it's still a work in progress - everything but the update hook is complete.

CommentFileSizeAuthor
#17 3153550-17-vote-entity.patch4.11 KBtr
vote-base-fields.patch4.11 KBtr

Issue fork votingapi-3153550

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

TR created an issue. See original summary.

tr’s picture

tr commented
Related issues: +#2898847: Vote list cache tag not invalidated when saving a vote, +#3153473: Making Votingapi Translatable

Note that #2898847: Vote list cache tag not invalidated when saving a vote is a related issue where again we aren't delegating to the parent, so we're losing functionality - cache tags aren't getting set properly.

Likewise, in #3153473: Making Votingapi Translatable, there is no need to completely redefine base fields etc. just to add language support, as language support will be inherited from the parent ContentEntityBase if we delegate the baseFieldDefinitions() to the parent properly.

tr’s picture

tr commented
Priority: Normal » Major
tr’s picture

tr commented
Related issues: +#2635224: ContentEntityBase should provide field definitions for key fields

Added related issue to link the core issue where ContentEntityBase acquired these base fields.

eelkeblok made their first commit to this issue’s fork.

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented

Correct me if I'm wrong, but when comparing the old code to that of the ContentEntityBase, I don't think there are actually any database changes needed (for a while there, I though the type column needed to change to bundle, but there actually is a mapping in the annotation). Of course, this will need to be tested - I am currently waiting on a conversion script to turn ~330.000 flags into votes, I guess that should be sufficient - but I am just wondering if I am overlooking something.

tr’s picture

tr commented
Version: 8.x-3.x-dev » 4.0.x-dev
Priority: Major » Critical

tr changed the visibility of the branch 8.x-3.x to hidden.

tr changed the visibility of the branch 3153550-vote-entity-unnecessarily to hidden.

tr changed the visibility of the branch 4.0.x to hidden.

tr opened merge request !30

tr closed merge request !30

tr opened merge request !31

tr closed merge request !31

tr changed the visibility of the branch 3153550-vote-entity to active.

tr changed the visibility of the branch 3153550-vote-entity to hidden.

tr’s picture

tr commented
StatusFileSize
new 3153550-17-vote-entity.patch4.11 KB

I screwed up the issue fork by trying to rebase the changes into 4.0.x. Maybe someone can fix that - I've tried all the simple things and am unwilling to spend more time on it.

So right now I will be relying on the patch instead. Here is a manual re-roll against 4.0.x.

tr changed the visibility of the branch 3153550-vote-entity-unnecessarily to active.

tr’s picture

tr commented
Status: Needs work » Active
guido_s’s picture

guido_s
Location Cologne
commented
Related issues: +#3396329: [meta] Alpha 4.x D11-compatible release, +#3451390: Automated Drupal 11 compatibility fixes for votingapi, +#3511188: Create a Drupal 11-ready release for Voting API
guido_s’s picture

guido_s
Location Cologne
commented

I just upgraded to 4.0.x-dev Version on a platform where I'm using this module with the Rate module with thumbsup rating and stars in entities (nodes and commerce products), views and webforms and had no issues so far without any database updates.

Upgrading drupal/votingapi (3.0.0-beta5 => dev-4.0.x 010514d)

Would be nice to have a tagged release for security / stability reasons.
When I got @tr right in the D11 issues of this module, this issue here is the reason why there is no tagged D11 release yet.
But I don't see what isn't working and if there really is anything left that needs to be done for this.

guido_s’s picture

guido_s
Location Cologne
commented

Even with the patch from this issue I agree with @eelkeblok that it wouldn't require any db updates.

Also asked the AI:

no update hook needed.

Why:

* The patch only switches to using the parent base field definitions and replaces t() calls with TranslatableMarkup + some label/description text tweaks.
* It does not add or remove any stored fields, indexes, or change any field storage settings.
* The id, uuid, and bundle type fields now come from the parent, but their storage characteristics remain the same (integer unsigned id, UUID readonly, bundle reference to vote_type). The patch just overrides their labels/descriptions.

What to do after applying:

* Just rebuild caches (drush cr). There’s no hook_update_N() or post-update required because no schema changes are introduced.

elaman’s picture

elaman
Primary language English
Location United States
commented

I did a manual upgrade-path validation using a real module that depends on Voting API: Voting API Reaction.

Test path

  1. Created a Drupal 9 site with DDEV using PHP 8.1.
  2. Installed Drupal 9 with drupal/votingapi and drupal/votingapi_reaction.
  3. Installed the site and enabled votingapi_reaction.
  4. Added a Reaction field to the Article content type.
  5. Created sample article content.
  6. Added reactions from multiple users.
  7. Upgraded the site to Drupal 10.4.
  8. Ran database updates and cache rebuild.
  9. Confirmed existing reaction/vote data persisted and voting still worked.
  10. Changed the DDEV PHP version to 8.3.
  11. Upgraded the site to Drupal 11.
  12. Ran database updates and cache rebuild.
  13. Confirmed existing reaction/vote data persisted and voting still worked.

Commands used

ddev config --project-type=drupal9 --docroot=web --create-docroot --php-version=8.1
ddev composer create-project "drupal/recommended-project:^9"
composer config minimum-stability dev
ddev composer require drush/drush drupal/votingapi drupal/votingapi_reaction
ddev drush si
ddev drush en votingapi_reaction

# Manual site setup:
# - Added a Reaction field to Article.
# - Created sample article content.
# - Added reactions from multiple users.

ddev composer require 'drupal/core-recommended:^10.4' 'drupal/core-composer-scaffold:^10.4' 'drupal/core-project-message:^10.4' 'drush/drush:^12'
ddev drush updb -y
ddev drush cr

ddev config --php-version=8.3
ddev restart
ddev composer require 'drupal/core-recommended:^11' 'drupal/core-composer-scaffold:^11' 'drupal/core-project-message:^11' 'drush/drush:^13'
ddev drush updb -y
ddev drush cr

Result

The Voting API data survived the Drupal 9 -> Drupal 10.4 -> Drupal 11 upgrade path in this test. Voting API Reaction continued to read and write votes after the upgrade.

I did hit an unrelated update issue around the missing help_search_items table during the Drupal 11 update. Creating that table allowed updates to continue, and it did not appear related to Voting API.

Notes

This is manual validation, not a substitute for automated update-path test coverage. However, it suggests that the current Voting API 4.x development branch can preserve existing vote data through a real upgrade path for at least this Voting API consumer.

eelkeblok opened merge request !66

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented

That last MR *might* be a good starting point, but apparently there are merge conflicts, that branch *is* really old. IIRC, this sounds scarier than it is, really.

steven jones’s picture

steven jones commented
Assigned: Unassigned » steven jones
Status: Active » Needs work
steven jones’s picture

steven jones commented
Status: Needs work » Needs review

Manually reviewing this and yeah, the base fields are going to be almost identical before/after this change, I think the only difference will be that technically before the 'type' base field wasn't required, but now is. So I should probably double check that that's not going to get flagged up problematically.

steven jones’s picture

steven jones commented
Status: Needs review » Reviewed & tested by the community

Did an install with the 4.0.x version, and then applied the MR, and the changes were totally fine, the SQL for the table is identical.
I don't see that there will be any Drupal API issues with making the type field required, that'll be fine (famous last words.)

steven jones’s picture

steven jones commented
Status: Reviewed & tested by the community » Fixed

Committed this change in! So close to getting a 4.0.x beta now!

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.

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented

🙌 Awesome.

Status: Fixed » Closed (fixed)

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