Make JS fallback wrapper conditional (as global setting)

We won't implement this, until we need it, but are happy to review MRs and discuss this.

I think this would be a nice job and exercise for @LRWebks! :)

I'm on it! :)

Just to have this straightened out:
These options' functionality doesn't exist in the code so far, right? So the functionality should be implemented as well?

Yes, correct! The JS fallback wrapper (.photoswipe-gallery .photoswipe-gallery--fallback-wrapper) exists, but not the different ways to handle it. Currently, it's applied always the same way.

I haven't digged into the PhotoSwipe architecture a lot so far, please tell me if you know: Where in the code is the point, that PhotoSwipe adds the fallback wrappers and I have to add the other options?

Found it!

Pushed my progress so far, have no idea on how to check if it works, though, since fallback wrapper usage is quite rare, and I can't see it in action. It would be appreciated if you were to review this anyways. :)

@LRWebks: Nice! :) I left some comments, please start with the #states API which has the largest learning effect for you. See the links I left.

Done again with what we have so far.

@LRWebks: VERY COOL, that was fast!! :)

Thought about the "default" (string) value again and I think we should use NULL instead for the default, instead of the custom string. Coud you please finally change that?

Afterwards, please sum up here, what's the status and what's left to be done here and assing the issue for review to me.
As this has minor priority, I will do the final review and provide final details when I have the time (weeks).

Well done Leon! :)

Changed the default value of the setting to NULL and also added the according values to the SettingsTest. Only thing left to do now would be to actually test the functionality of the implementation, preferably by code - but as the usage of fallback wrappers is very rare if you do things right in PhotoSwipe, I currently don't know how to test it.

Thanks @LRWebks! Assigning this to me for now, as written in #15. So far, so good! :)

@Grevil: As you're just on it, please have a quick look. Afterwards re-assign to me. Has no priority.

The phpunit pipeline is currently failing, see https://git.drupalcode.org/issue/photoswipe-3385637/-/pipelines/28414/te....

Only thing left to do now would be to actually test the functionality of the implementation, preferably by code - but as the usage of fallback wrappers is very rare if you do things right in PhotoSwipe, I currently don't know how to test it

As for manual tests, I think you could test the fallback on fields using the photoswipe formatter, if you comment out line 320 of "/src/Plugin/Field/FieldFormatter/PhotoswipeFieldFormatter.php". This is where the gallery wrapper is set. If the wrapper isn't there, the fallback should execute.

As for Functional tests, you can take a look at the "photoswipe_twig_extension_test" test module in "/tests/modules/photoswipe_twig_extension_test". Add another template similar to "photoswipe_twig_extension_test.function_test.html.twig", but without the attach_photoswipe() twig function call and the gallery wrapper, and then programmatically enable the "photoswipe_always_load_non_admin" photoswipe setting. The fallback wrapper should then appear.

I don't know if there is anything we can do about the failing JS test, seems to be a problem with the waitForElementVisible() function again...

This is really weird...

The test failure is definitely unrelated, the same test also fails if running 5.x dev locally. For some reason, re-executing the test job on 5.x remotely doesn't throw this test error.

I checked the test with the applied MR, and it is definitely correct. The HTML structure of the HTML Test output is also as expected and behaves correctly. I tinkered a bit around with the css selectors but still same test failure, no idea...

Let it fail for now, if it still fails later after merge, we need to create a follow-up issue and I can take a deeper look, once we have the time.

@LRWebks, please continue with #22 for now.

I might have found the problem? As I identify the problem to reside within the waitForElementVisible() function, it might be the same problem as already fixed in this older issue ( https://www.drupal.org/project/photoswipe/issues/3387336 🐛 Fix assertion issue in automated tests Fixed )? I think in this case there actually is no assertTrue() wrapper around the function!

@LRWebks please implement it and give it a try! Wondering, why we didn't see that in the other issue :(

Ah, now I know what the problem is, and I'm on it to fix it!
What we are parsing to the $this->assertEquals() is:

assertEquals(theSettingsValue, ourExpectedValue);

However, the function actually requires the parameters to be in the order:

assertEquals($expected, $actual);

(from the documentation)
So the whole parameters are switched-around incorrectly!
As far as I'm aware, this might also be the case in other PhotoSwipe tests and other projects, so we should be on the lookout!

@LRWebks: Thanks, of course it's important to have the parameters in the correct, expected order. But this won't change failing tests, as it's a boolean comparison (which has the same logical result despite its order).

What about your point from #25?

Yeah, unfortunately, you're right...
The waitForElementVisible() functions already have the $this->assertNotNull() wrap around them, so now it's an absolute mystery why this test fails, and why in a line as random as Line 136, not the first assertion but a random one I didn't even touch...

Okay then let's stop investing further time here. This becomes too expensive. If anyone has an idea, just try and post an update, please.

I can at least share a new discovery before putting this aside for now: Going back intro the settings within the test instead of the profile node reveals that actually none of the settings are changed at all!
The reason the first check doesn't fail but the second one does, is that the first field has a default value of FALSE yet it was being unchecked instead of checked and then compared to FALSE once again for some reason...
I have at least fixed that little issue right now.