Contrib.social

[5.x] Separate image and entity_reference formatters

Open on Drupal.org →
Created on 4 August 2022, about 2 years ago
Updated 20 June 2024, 3 months ago

Problem/Motivation

This is a proposal to discuss, originating from #3226541: Respect access permissions → :

Current field formatter implementation uses the same field formatter class for image and entity_reference:

/**
 * Plugin implementation of the 'photoswipe_field_formatter' formatter.
 *
 * @FieldFormatter(
 *   id = "photoswipe_field_formatter",
 *   label = @Translation("Photoswipe"),
 *   field_types = {
 *     "entity_reference",
 *     "image"
 *   }
 * )
 */

and

/**
 * Provides formatter that supports responsive image.
 *
 * @FieldFormatter(
 *   id = "photoswipe_responsive_field_formatter",
 *   label = @Translation("Photoswipe Responsive"),
 *   field_types = {
 *     "entity_reference",
 *     "image"
 *   }
 * )
 */

This leads to a lot of if / else in the formatter and we can't use EntityReferenceFormatterBase for example, which would provide perfectly implemented access checks for example.

So I don't think it was a clever choice to combine both in the same formatter.

For 5.x we should split them up so that the formatters become a lot smaller and easier to understand. Less prone to errors and things like permission checks can be inherited. Eventually introduce a PhotoswipeFormatterTrait for shared methods between them.

Steps to reproduce

Proposed resolution

Separate the formatters for image and entity_reference
Eventually introduce a PhotoswipeFormatterTrait for shared methods

Remaining tasks

User interface changes

API changes

Data model changes

✨ Feature request
Status

Needs work

Version

5.0

Component

Code

Created by

🇩🇪Germany Anybody Porta Westfalica

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment over 1 year ago →
    🇩🇪Germany Anybody Porta Westfalica
  • First commit to issue fork.
  • Merge request !116Issue #3301760: [5.x] Separate image and entity_reference formatters → (Open) created by Unnamed author
  • Pipeline finished with Failed
    8 months ago
    Total: 172s
    #87355
  • Status changed to Needs review 8 months ago
  • Comment 8 months ago →
    🇺🇦Ukraine bobi-mel

    I've separated mage and entity_reference formatters. Please check it.

  • Comment 8 months ago →
    🇺🇦Ukraine dinazaur

    Hi @Anybody just to clarify. What we need is to implement 4 formatters - 2 for image (Responsive and Not) 2 for entity_reference (Responsive and Not) Is that correct?

  • Comment 8 months ago →
    🇩🇪Germany Anybody Porta Westfalica

    @dinazaur:

    Hi @Anybody just to clarify. What we need is to implement 4 formatters - 2 for image (Responsive and Not) 2 for entity_reference (Responsive and Not) Is that correct?

    Yes that's right. Of course we should try to not duplicate code where possible, for example by using inheritance and base / abstract classes.

  • Pipeline finished with Failed
    8 months ago
    Total: 203s
    #89714
  • Pipeline finished with Failed
    8 months ago
    Total: 298s
    #89720
  • Pipeline finished with Failed
    8 months ago
    Total: 253s
    #89776
  • Assigned to bobi-mel
  • Status changed to Needs work 8 months ago
  • Comment 8 months ago →
    🇺🇦Ukraine bobi-mel
  • Pipeline finished with Success
    8 months ago
    Total: 248s
    #90262
  • Pipeline finished with Failed
    8 months ago
    Total: 266s
    #90800
  • Pipeline finished with Success
    7 months ago
    Total: 259s
    #106799
  • Pipeline finished with Success
    7 months ago
    Total: 318s
    #108104
  • Issue was unassigned.
  • Status changed to Needs review 7 months ago
  • Comment 7 months ago →
    🇺🇦Ukraine bobi-mel

    I've separated mage and entity_reference formatters. Please check it.

  • Status changed to RTBC 3 months ago
  • Comment 3 months ago →
    🇺🇦Ukraine vlad.dancer Kyiv

    Hey @bobi-mel, thanks for your work! I reviewed it, looks great! I've worked on #3455963 on top of your code.

  • Status changed to Needs work 3 months ago
  • Comment 3 months ago →
    🇩🇪Germany Anybody Porta Westfalica

    Thank you all very much! To ensure this works stable, we should have tests for the new features like for the existing old ones.
    This module is widely used and broke too often in the past. We shouldn't risk that. Thanks!

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024