When using PhotoSwipe as field formatter with an image style set, the dependency on the image style configuration is missing, when you export the entity view display configuration. This may lead to problems when e.g. packaging pre-defined configurations in a custom module or installation profile. Drupal then may install the configuration entities in a wrong order because it wasn't informed correctly about the dependencies
I've reported the exact same problem a couple of month ago in Colorbox module, where the problem was already solved. You can use that as a blueprint for this module as well: #2686805: Image style dependency is missing on configuration export
I don't have time to write a patch at the moment, but possibly in a couple of days (maybe friday...), but maybe another one is quicker than me ;)) :)
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 2856418-7.patch | 2.89 KB | agoradesign |
Comments
Comment #2
anybodyCould you please retry the latest dev and re-roll the patch against it, if the issue is not fixed there yet?
Thanks!
Comment #3
anybodyComment #4
anybody@agoradesign are you still active and could re-check that with latest dev?
Comment #5
agoradesign commentedHi.
I'm still using the module very often but honestly did not pay too much attention to the exported configuration these days.
It's already a couple of days ago you asked me to try the latest dev. Meanwhile this would be equal to just use the latest beta, right?
So I just went to a site, where I recently updated to beta5 and re-saved an entity view display of a node, where the photoswipe formatter is used. There we use the image style named "product" as photoswipe_node_style -> the image style is not part of the config dependencies. So, issue isn't fixed yet... unfortunately I never had time to write a patch for this
Comment #6
anybodyOk thank you for your feedback. Let's hope for someone who has the time to check this and create a patch. I won't have the time in the next months.
Comment #7
agoradesign commentedI've finally found time to write a patch :)
Comment #9
agoradesign commentedok, here's the reason for the failing tests: #2987299: Coding standards violations in photoswipe.theme.inc let test runner fail
Comment #10
anybodyRetesting with fixed coding standards issue.
Comment #12
anybody@agoradesign, your solution looks perfect. Have you tested the replacement functionality manually?
Of course it will make sense to write tests for this, especially the replacement functionality.
But I think we don't have to wait until tests are written because the functionality hasn't existed before in this module and I think it won't make things worse. Furthermore the module doesn't have any tests at all yet. Anyway if someone can find the time to write tests, that would be great and very helpful!
Comment #13
anybodyComment #15
agoradesign commentedYes, I've tested it manually and it worked as expected. The logic is taken from Colorbox module (as mentioned in the summary), so this part should be solid.
You're right. Having test woudl be great + only fixing bugs with a new test included as well.. but I know the dilemma because we all write the modules in spare time and/or as nice side effect of any client work, and in both cases there's always too less time :(
Comment #16
anybodyYes, same for me... I'm asking myself why the test are still failing with latest dev... has there been a testbot /ci update which fails if no tests are existing?
Comment #19
anybodyThank you very much for your great work :)
Commited to dev and created a new beta-6. Please test.
Comment #20
agoradesign commentedYou're welcome :) i believe it's the duty of us devs to help fixing bugs in great OSS projects :)
don't know if there's a problem with the testbot not having tests at all.. would be sad because the coding standards check is still useful