Photoswipe Caption: Selected source field is printed including the field label (no matter if the label is turned off in the field configuration)

Thanks, that looks like a bug in the implementation. I guess it should never ever render the label.

Agreed, getting the field->getValue() or field->value should fix this.

@thomas.frobieter exactly. I implemented that, please try if it returns the expected results.

This is kind of a breaking change, but the previous implementation doesn't make a lot of sense to me and we shouldn't make this more complicated by allowing to select a view mode etc.
From my perspective the value is what we want.

@Grevil: Please review and checked if the output runs through Twig. Otherwise, we need to ensure that the value is properly escaped for security reasons!

I added a second option, which might be more intentional, as I saw the lines before. It looks to me as if the view mode parameter was entirely forgotten before, so the field was always rendered in full?

Not yet sure how to proceed here perfectly.
a) Use MR104
b) Use MR105
c) Use MR105 and add another view mode for "field value" and rename the existing to "field in viewmode" or sth. like that.

Opinions?

That's a good point... if this relies on the view mode configuration, what happens if you don't want to show the caption field outside the pswp layer? in other words, if the field display is disabled.

What about using the (escaped) raw value? The only case I can think of where a field formatter makes sense for the caption field is to use something like smart-trim to shorten the text. BUT, this also has the downside that the configuration is the same as the regular output of the caption field when printed elsewhere.

So +1 for using the escaped raw value from the entity, not the one from the render array.

@Grevil:

I'd vote for MR!104 plus
- security check (see #6)
- Removal of the strange view mode code above
- Use a common prefix like "Field value: " + human readable field name + (machine name) if not too complicated
- Set title as default value for this selection

Rendering the field seems super strange and incomplete this way, as the selected field might
- be hidden or configured differently in the default view mode
- have a label (like described in the IS)
- have complex HTML

So the label is the better solution.

Please also have a look at code changes (blame) when this was introduced and wny... the history of this change might be interesting.

Was this already existing like this in 4.x?

MR!104 seems fine, I will check the points you gave.

But we should check the original issue, this was added in.

Was this already existing like this in 4.x?

Yes.

Please review and checked if the output runs through Twig

It gets escaped based on the field provided. For example, if we use the body WYSIWYG field, and use "FULL HTML", nothing is escaped and we can "inject" scripts in the caption. If we use "Restricted HTMl" it gets escaped. Normal field values do get escaped, so I get we need additional escaping here.

Don't quite understand a few points, but please review! Should be all set now!

Anybody → closed merge request !105

@Grevil: I left comments, please see.

Don't quite understand a few points

Which?

- Use a common prefix like "Field value: " + human readable field name + (machine name) if not too complicated
- Set title as default value for this selection

Rendering the field seems super strange and incomplete this way, as the selected field might
- be hidden or configured differently in the default view mode
- have a label (like described in the IS)
- have complex HTML

This.

Use a common prefix like "Field value: " + human readable field name + (machine name) if not too complicated

I meant the field display select options. Currently they show the field machine name. If simple, we should improve it like described Prefix + HR + MR names (see above)

Set title as default value for this selection

Separate issue now: 📌 Make the default Caption source 'title' (not 'alt') Needs review

Rendering the field seems super strange and incomplete this way, as the selected field might
- be hidden or configured differently in the default view mode
- have a label (like described in the IS)
- have complex HTML

So the label is the better solution.

Fixed by MR!104

@Grevil: Clearer now?

yes

Ok, please review once again @Anybody!

Commented finally. Afterwards RTBC + Merge, as these are only textual changes.

Alright, tiny code adjustment, good thing tests exist! 😅

Final review through the adjusted test :)

I am going insane, with these freaking field formatter third party settings!!!!

Should be fixed now, please review!

PHPCS is complaining (and eslint but I think that's old)

And one unresolved comment.

The phpcs issue was also related to another issue, but fixed anyway!

Alright, all green now! I accidentally skipped the pipeline and executed it manually afterwards. Unfortunately the UI shows it still as "skipped", see here for the pipeline output: https://git.drupalcode.org/issue/photoswipe-3392976/-/pipelines/29389.

RTBC! :)

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