Contrib.social

[5.x] Separate image and entity_reference formatters

Open on Drupal.org →
Created on 4 August 2022, over 2 years ago
Updated 4 February 2024, 11 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.
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
    11 months ago
    Total: 172s
    #87355
  • Status changed to Needs review 11 months ago
  • Comment 11 months ago →
    🇺🇦Ukraine bobi-mel

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

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

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

  • Status changed to RTBC 6 months ago
  • Comment 6 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 6 months ago
  • Comment 6 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!

  • Status changed to Needs review about 2 months ago
  • Comment about 2 months ago →
    🇺🇦Ukraine bobi-mel

    We have separate tests for the Image and the Media Reference. While working on the issue I fixed the test for the Media Reference after splitting the formatted

    I guess they are enough

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024