PHPCS ensures that coding standards are followed consistently across a project. This makes it easier for developers to read and find the relevant code.

There are a number of issues identified by PHPCS that should be cleaned up.

CommentFileSizeAuthor
#6 Screenshot from 2024-09-27 12-42-56.png123.51 KBriddhi.addweb
#6 Screenshot from 2024-09-27 12-42-30.png121.36 KBriddhi.addweb
#6 Screenshot from 2024-09-27 12-42-03.png114.4 KBriddhi.addweb

Issue fork legal-3471496

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:

About issue forks

Comments

john franklin created an issue. See original summary.

john franklin opened merge request !22

john franklin’s picture

john franklin commented
Status: Active » Needs review

There is one test failing in the pipelines. The same test fails in other recent MRs.

john franklin opened merge request !23

john franklin’s picture

john franklin commented

The "test-pipelines" MR is 3.0.x, minus the LICENSE.txt file. The unit tests fail in exactly the same way when running against 3.0.x as they do the fix-phpcs-issues MR, so I consider the MR to be valid.

riddhi.addweb’s picture

riddhi.addweb commented
Status: Needs review » Needs work
StatusFileSize
new Screenshot from 2024-09-27 12-42-03.png114.4 KB
new Screenshot from 2024-09-27 12-42-30.png121.36 KB
new Screenshot from 2024-09-27 12-42-56.png123.51 KB

I tried to apply both MR [22, 23] and it applied cleanly, but still, the errors are displayed. Still, PHPCS errors are there and not resolved. Attaching the SS for the same.

john franklin changed the visibility of the branch 3471496-test-pipelines to hidden.

john franklin’s picture

john franklin commented

MR 22 is the correct one. I've hidden 23 and deleted the branch from git. MR 22 passed PHPCS in the pipelines. I'll rerun it and see what comes back.

Edit: Gitlab won't allow me to delete the MR 23 branch.

john franklin opened merge request !24

kul.pratap’s picture

kul.pratap commented
Assigned: Unassigned » kul.pratap

Working on it.

kul.pratap’s picture

kul.pratap commented
Assigned: kul.pratap » Unassigned
Status: Needs work » Needs review

Pipeline passed with no warning and errors.

john franklin’s picture

john franklin commented

Thanks for the updates @kul.pratap. A completely green pipeline is a lovely thing to see! I updated the cspell handling to inject unknown words in the `.gitlab-ci.yml` file instead of recreating the whole .cspell.json file, and backported the changes to the 3.x branch.

@robert castelo, there are two MRs attached here:

MR 22 - Applies updates to fix the "validate" class of tests in the pipeline for 3.0.x. This is as close as we're going to get to green until that last phpunit test is fixed in 3.0.x.

MR-24 - Applies updates to fix the "validate" class of tests in the pipeline for 4.0.x. This brings the pipeline to full green.

I don't think it's appropriate for me to RTBC this as I'm one of the people contributing the fixes. @riddhi.addweb, can you check on 3.x or 4.x?

arunsahijpal’s picture

arunsahijpal commented
Status: Needs review » Reviewed & tested by the community

Great Work @kul.pratap and @john-franklin,
I've tested MR!22 and MR!24 on my local and phpcs doesn't reports any error, and phpcs is passing in the pipeline too!
Hence moving it to RTBC.

john franklin’s picture

john franklin commented

@robert-castelo, can you review and merge these two MRs, one each for the 3.x and 4.x branches, please?

orkutmuratyilmaz’s picture

orkutmuratyilmaz
Primary language Turkish
Location İzmir
commented

any chance for merge?

robert castelo made their first commit to this issue’s fork.

robert castelo’s picture

robert castelo commented
robert castelo’s picture

robert castelo commented

Why is "behaviour" (UK spelling) being changed to "behavior" (US spelling)?

Not going to hold up merging, but feel like both should be allowed.

robert castelo’s picture

robert castelo commented
Status: Reviewed & tested by the community » Fixed
orkutmuratyilmaz’s picture

orkutmuratyilmaz
Primary language Turkish
Location İzmir
commented

@robert castelo thank you:)

Status: Fixed » Closed (fixed)

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

john franklin’s picture

john franklin commented

Why is "behaviour" (UK spelling) being changed to "behavior" (US spelling)?

Because Drupal uses American spelling rules. It does feel rather arbitrary.

mxh’s picture

mxh commented

The commit into 3.0.x breaks the admin settings form. Was AI used to produce this "cleanup"?

https://git.drupalcode.org/project/legal/-/blame/3.0.x/src/Form/LegalAdm...
- "entity_type.manage" does not exist as a service (typo)
- "role" entity type does not exist. Correct entity type ID is "user_role".

Such changes should have at least been manually tested one time before setting RTBC.