Problem/Motivation

The biggest part of the issues this module has are caused by its custom field storage implementation. This also adds a lot of unnecessary complexity to the module. It's reinventing the wheel, since Drupal has a whole field storage system built in that automatically deals with things like translations and revisions.

Proposed resolution

Rewrite the module to stop using the custom storage service. Instead, make the caption a standard field property whose storage is handled like any other field property in Drupal.

To address these issues, the module should be refactored to eliminate the custom storage service and instead leverage Drupal's default field storage system. This involves redefining the caption as a standard field property, allowing Drupal to handle its storage, translations, and revisions natively. By doing so, the module will:

  1. Simplify the codebase: Remove the custom storage implementation and rely on Drupal's proven field storage mechanisms.
  2. Improve maintainability: Reduce the risk of bugs and inconsistencies by using core functionality.
  3. Enhance compatibility: Ensure seamless integration with other Drupal features, such as translations and revisions, without additional custom logic.
  4. Follow best practices: Align with Drupal's architecture by utilizing its built-in field storage system.

Implementation Steps

  1. Remove Custom Storage Service:
    • Delete the custom storage service and related classes.
    • Ensure all dependencies on the custom storage are removed.
  2. Define Caption as a Field Property:
    • Modify the field type plugin to define the caption as a standard field property.
    • Update the schema and property definitions to reflect this change.
  3. Update Widget and Formatter:
    • Adjust the field widget to handle the caption property as part of the default field storage.
    • Update the field formatter to display the caption property correctly.
  4. Migrate Existing Data:
    • Provide an update hook or migration path to transfer existing caption data from the custom storage to the default field storage.
  5. Test Thoroughly:
    • Ensure the changes work seamlessly with translations, revisions, and other Drupal features.
    • Verify that existing data is preserved and displayed correctly after the migration.

Remaining tasks

Provide an upgrade path.

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:

Comments

dieterholvoet created an issue. See original summary.

dieterholvoet’s picture

Issue summary: View changes
Status: Active » Needs work

The minimum Drupal version is bumped to 9.2 because of the usage of the usage of the new field widget hook. I also added back the Image module dependency since it was incorrectly removed in #3355337: Support Drupal 10. Setting to Needs work for the upgrade path implementation.

bachbach’s picture

It's reinventing the wheel, since Drupal has a whole field storage system built in that automatically deals with things like translations and revisions.

exactly what i though after hours trying to make caption in nested parragraph translation to work :(
then i found your issue and your patch work like a charm!

had to :
pmu the module (obviously delete all the caption contents)
composer require dev version of module
apply your patch
enable the module again

thanks for your work !

bachbach’s picture

Priority: Normal » Major
foxy-vikvik’s picture

Assigned: Unassigned » foxy-vikvik
foxy-vikvik’s picture

Issue summary: View changes
Issue tags: +refactoring, +field storage update
foxy-vikvik’s picture

Version: 2.0.x-dev » 3.0.x-dev
dieterholvoet’s picture

@foxy-vik why did you make the caption a plain text field instead of a formatted text field in #e2e9925? In the current version of the module it's also a formatted text field, so I don't see why you would change that.

foxy-vikvik’s picture

@dieterholvoet It was a mistake. I found an issue and changed it. This merge request is still a work in progress. So it will be formatted text. I will change it when I have time.

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

foxy-vikvik’s picture

Assigned: foxy-vikvik » Unassigned
Status: Needs work » Needs review
kksandr’s picture

Assigned: Unassigned » kksandr
Status: Needs review » Needs work
kksandr’s picture

Assigned: kksandr » Unassigned
foxy-vikvik’s picture

Status: Needs work » Needs review

foxy-vikvik’s picture

Version: 3.0.x-dev » 4.0.x-dev
Issue summary: View changes
Status: Needs review » Fixed
Issue tags: -field storage update

Committed to 4.0.x. Thanks!

dieterholvoet’s picture

Maybe it could be an idea to not drop the old tables just yet, just in case there are still issues with the migration? The dropping of the tables could be added once a stable release of the module is made.

kksandr changed the visibility of the branch 3472997-rewrite-module-to to hidden.

kksandr’s picture

That's a good idea, let's do it that way.

  • foxy-vikvik committed 0eec0608 on 4.0.x authored by kksandr
    Issue #3472997 by kksandr, foxy-vikvik, dieterholvoet: Kept legacy...

kksandr changed the visibility of the branch 3472997-fixes to hidden.

  • kksandr committed 310c739b on 4.0.x
    Issue #3472997 by kksandr: Refactor hooks to OOP
    

  • kksandr committed 9679b536 on 4.0.x
    Issue #3472997 by kksandr: Make image field config safe to use
    

kksandr changed the visibility of the branch 3472997-fixes-2 to hidden.

  • kksandr committed f4a3cd12 on 4.0.x
    Issue #3472997 by kksandr: Fix schema cleanup on uninstallation
    

kksandr changed the visibility of the branch 3472997-fixes-3 to hidden.

  • kksandr committed c85834f6 on 4.0.x
    Issue #3472997 by kksandr: Fix post-update hook
    

Status: Fixed » Closed (fixed)

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