Problem/Motivation

When the photoswipe formatter is used in entity reference fields the image alt/title tag, as photoswipe caption, not working.

Steps to reproduce

1) Add an entity reference field (media)
2) Go to Manage display and select:
Format => Photoswipe
Photoswipe Caption => Image alt tag or Image title tag

CommentFileSizeAuthor
#11 3258617-image-alttitle-tag.patch1.48 KBdwisnousky

Issue fork photoswipe-3258617

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

balis_m created an issue. See original summary.

balis_m’s picture

Status: Active » Needs review
balis_m’s picture

balis_m’s picture

Version: 3.0.0 » 3.x-dev
marcoka’s picture

I have applied the patch and that fixes the referenced issue and now the formatter that i use in a display mode (/admin/structure/media/manage/image/display) works and the style is applied to the images i added inline using the ckeditor mediebutton with that display mode.
Thank you.

anybody’s picture

Issue tags: +Needs tests

Thank you all! My fear is that committing this fix breaks something else in other cases... So we please need test coverage for the future. Hope you understand, we're jumping from one problem into the next as the module is very widely used in different scenarios.

anybody’s picture

Status: Needs review » Needs work

This issue is blocked by missing test coverage for the photoswipe module.

Please first help to fix #2998694: Write tests for typical use-cases to reproduce this error, create a test and ensure the fix on this for all use-cases.
Hope you understand, we're jumping from one problem into the next as the module is very widely used in different scenarios.

See #2998694: Write tests for typical use-cases and help please. Until that, this issue is blocked.

anybody’s picture

Also see the comment from @Berdir in

https://www.drupal.org/project/photoswipe/issues/3132556#comment-14255411:

I also noticed the alt/title logic. That is clearly Drupal 7 code (those fields were/are defined by the file_entity module and the way the field value is accessed will not do anything in Drupal 8+, but I resisted the urge to clean that up.

anybody’s picture

Status: Needs work » Fixed

Thank you very much, I just reviewed and tested this and committed to 3,x-dev. Please try 3.x-dev now :)

dwisnousky’s picture

StatusFileSize
new1.48 KB

balis_m’s picture

Status: Fixed » Needs review

@Berdir is right.

The check for the field_file_image_title_text is not needed anymore. I've created a new merge request.

anybody’s picture

Thank you @balis_b, I'll have a look soon!

@dwisnousky what's that patch?

anybody’s picture

@balis_m: Thank you, looks good! I replaced the !empty check by ternary operator, see: https://stackoverflow.com/a/49464633/10031165

Could you please add tests for checking the alt and title tag as of #2998694: Write tests for typical use-cases? We have to stop introducing regressions to this module due to missing tests.

anybody’s picture

Still needs review, as I made changes, I can not review myself. Please also test manually.

balis_m’s picture

After a manual test, I confirm that it works well.

anybody’s picture

Status: Needs review » Fixed
Issue tags: -Needs tests

Thank you @balis_m, let's get this committed to dev. :) Great work!

Status: Fixed » Closed (fixed)

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