Problem/Motivation

Would be great to fix the code style issues and get the pipeline green again. Should we raise some of them to errors?

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork photoswipe-3620906

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

anybody created an issue. See original summary.

anybody’s picture

Issue summary: View changes

ressa made their first commit to this issue’s fork.

ressa’s picture

Status: Active » Needs review

I took a look at it @anybody, and the phpcs errors, and cspell suggestions I could fix.

It said that a component in photoswipe/modules/photoswipe_dynamic_caption/photoswipe_dynamic_caption.module was no longer used:

----------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------------------
 8 | ERROR | [x] Type Drupal\Component\Utility\Xss is not used in this file.
----------------------------------------------------------------------------------------------------------------------

... and it does seem use of Xss::filter was removed here:

-          // The caption value needs to get escaped:
-          $caption = Xss::filter($fieldValue);
+          // Assign the field value to the caption:
+          $caption = $fieldValue;

From "Improve caption value output"
https://git.drupalcode.org/project/photoswipe/-/commit/b70e9a47ca1174644...

But probably best to double check this change in the MR.

There are also some more complicated tasks, like warnings about converting \Drupal::service to a dependency injection, which is a bit beyond my basic coding skills:

$ ddev exec ./vendor/bin/phpcs --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml --standard=Drupal,DrupalPractice web/modules/contrib/photoswipe -v
[...]
Processing photoswipe.theme.inc [209 tokens in 43 lines]... DONE in 4ms (0 errors, 0 warnings)
FILE: /var/www/html/web/modules/contrib/photoswipe/src/Form/PhotoswipeSettings.php
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------
 433 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
----------------------------------------------------------------------------------------------
[...]

It seems like the phpcs and cspell tests are now green, but like you wrote, perhaps some more work is needed here?

anybody’s picture

Assigned: Unassigned » grevil
Status: Needs review » Needs work

Thank you very much! We should try to make eslint and phpstan also happy if possible or add ignores maybe?
Asking @grevil to take a short look if possible.

grevil made their first commit to this issue’s fork.

grevil’s picture

Issue tags: -Novice

Not really a novice task, as the failures need some major refactoring.

grevil’s picture

Assigned: grevil » anybody

All green now!

Thanks ressa for your initial work! I created a seperate branch, since this needed some further work, and some changes weren't necessarily needed (e.g. README adjustments and the @codingStyleIgnoreX removals).

Please review @anybody!

ressa’s picture

Assigned: anybody » Unassigned
Status: Needs work » Needs review

Thank you @grevil! Nice to see all tests are now green :)

ressa’s picture

Sorry about the change in the "Assigned" field, I am not sure why that happened?

I only changed the Status to "Needs review", but maybe that affected that field as well? (and I cannot add @anybody)

anybody’s picture

Status: Needs review » Reviewed & tested by the community

Thank you both very much! Merging!

anybody’s picture

Status: Reviewed & tested by the community » Fixed

@grevil: As follow-up finally please see the both other tasks assigned to you, so we're finished here. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • anybody committed bd792cc9 on 3605632-drushcommands-revised authored by grevil
    task: #3620906 Fix code style issues