Closed (fixed)
Project:
Image Delta Formatter
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
29 Sep 2020 at 21:11 UTC
Updated:
9 Dec 2023 at 14:38 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
aleixjust with few adaptations it seems it's working with media fields.
Comment #3
neograph734Confirming that this works for me as well. Thanks for your effort.
I am just wondering if the plugin name
media_delta_formattercould cause name clashing issues in the future? Perhapsimage_delta_formatter_mediawould be safer?Comment #4
fgmComment #5
neograph734I've been using for quite a while without any issues. Switching to RTBC.
Comment #6
mitsuko commentedYes, it works very well.
A new version with this patch commited will be wonderful !
Comment #7
mitsuko commentedOne more thing...
If you apply the patch and install the module image delta formatter before media module is also enabled, you get the following error : Class 'Drupal\media\Plugin\Field\FieldFormatter\MediaThumbnailFormatter' not found in include() (line 21 of modules/contrib/image_delta_formatter/src/Plugin/Field/FieldFormatter/MediaDeltaFormatter.php
That sounds like Media become a dependency for image delta formatter.
Maybe move the this patch/new formatter in a module/submodule with dependency marked could be the solution...
Comment #8
neograph734@Mitsuko, good find. Changing the status because of that.
I am wondering if we could also do this with isApplicable. Could you perhaps test by adding the following to the class?
If this does not work, a sub-module might indeed be an option.
Comment #9
mitsuko commented@Neograph734, sorry, I can't apply the new patch, I get an error both with 8.x-1.1 and 8.x-1.x-dev.
Comment #10
neograph734Thanks for the fast feedback. The file had a file encoding issue. I believe this one should work better.
UPDATE
To be sure, you got the error when you visited the mange display page? Then I think the issue still exists with this patch...
Comment #11
mitsuko commentedYes the error occurs on display page and is still present with the last patch applied.
Thanks you for looking around this issue.
Comment #12
neograph734I was about to update the patch and make this into a sub-module, but then started looking around. It appears that there are already other modules solving the same issue. Note that I have not tested any of the modules yet, but I will in the near future.
https://www.drupal.org/project/single_image_formatter
https://www.drupal.org/project/single_media
https://www.drupal.org/project/entity_delta_formatter
Since this issue has not received any maintainer feedback I am wondering if we should not switch to one of the other solutions, rather then duplicating the functionality here once more?
Comment #14
mitsuko commentedI tried the first 2 on the list before discover and switch to Image Delta Formatter.
IMHO, Image Delta Formatter is more efficient and granular.
You are not limited to 1 image, and I like especially squeeze the media display mode to pick the image style in the parent's entity display mode.
Indeed, the issue not seems priority for maintainers ;-) but module and patch #2 make the job so far ;-)
I created an issue fork with the submodule approach but it's a little bit messy because the generated diff patch is against the 7.x branch instead 8.x...
Comment #15
mitsuko commentedThe good diff is here : https://git.drupalcode.org/issue/image_delta_formatter-3173945/-/merge_r....
I add the patch to the issue.
Comment #16
neograph734@Mitsuko, I see. I was not aware of that. Thanks for your efforts then :)
I still have the concern I've raised in comment #3 though. Especially now this is becoming a sub-module proper naming might be important?
Comment #17
mitsuko commented@Neograph734, yes, you're completely right, prefix the submodule name with main module name is good practice and more secure.
I changed the name according to your suggestion, fixed the target branch in the issue fork to 8.x-1.x, so the generated diff is ok.
Add a new patch nevertheless.
Comment #18
neograph734Looks good to me. After updating the patch and enabling the module I've had to update the display style because (obviously) the previous formatter no longer exists. From there all works as expected.
The only think I noticed is that "Linked to content" is displayed twice, but that appears to be the case with all formatters and must be something that is inhereted from a parent class. So believe that is unrelated.
Comment #19
imclean commentedJust to add to the list in #12, field limiter provides something similar.
https://www.drupal.org/project/field_limiter
Comment #20
neograph734Marking RTBC, been using this for half a year without any issues.
Comment #21
mitsuko commentedUpdate the patch for drupal 10 compatibility
Comment #22
fgm@mitsuko: could you apply that patch to the existing MR at https://git.drupalcode.org/issue/image_delta_formatter-3173945/-/tree/31... ? As it stands, the current MR does not include D10 support.
Also, more importantly, I think there is no reason to have separate modules to support that use case: we could just as well have the two plugins in the same module, and spare the extra load for an additional module.
In addition, since both plugins ultimately derive from the same underlying ImageFormatter (via MediaThumbnailFormatter in the case of image_delta_formatter_media), it would make sense to just reuse the code instead of duplicating it. One possibility would be to defined a Trait holding that common code, and reduce both plugins to one-liners using that trait.
Comment #23
neograph734Please also check comment #7. It is split because of the dependency on the media module.
Comment #24
fgm@neogrpah734 indeed, since media is a core module and has all but replaced image module for most common use cases since Drupal 8.4, I do not consider the dependency on media to be an issue any longer: this is no longer 2021.
I just pushed a combined version on the MR, which also drops D8 support since it has been unsupported for almost 2 years already: comments welcome.
Comment #25
fgmIn patch form in case anyone prefers that.
Comment #26
fgmComment #27
fgmWith the added dependency, the code is now missing a hook_update_N to enabled media if needed.
Comment #28
fgmAdded hook_update_N.
Comment #29
fgmThere was a typo in the media formatter name ("medial_delta_formatter").
Regarding the conditional loading with isApplicable, it couldn't work because to call the method, the class first needs to be loaded and that will fail if media.module is not loaded.
Another approach could be to compose the thumbnail formatter instead of inheriting from it, to support conditional loading, but it would make the code significantly more complex.
Comment #31
fgm@berdir suggested a smarter approach on Slack: the Media delta plugin is now conditional, which avoids both the extra submodule and depending on media.module and will still enable the Media formatter automatically if media is present.
I guess that's a better choice for everyone ?
Comment #33
fgmMerged that simplified version to current dev HEAD, thanks everyone.
Comment #35
neograph734@fgm thanks for all the work. Is there any chance you could create a tagged release with these changes included?
Comment #36
fgm@Neograph734 the plan is to do it after D10.2 release, along with #3399449: Update Drupal / PHP support. Think it's worth doing one before ?
Comment #37
neograph734Thanks for the fast reply! I believe that 10.2 is planned for next week or so? No need to create a new release now then.
I was only asking because I figured that I was still on the dev version.