Problem/Motivation

In #3605477: Better tests performance by avoiding data providers MR, we have applied php-cs-fixer with Drupal 8 rules on our tests: https://github.com/drupol/phpcsfixer-configs-drupal

It shows phpcs/phphcbf may not be enough to lint our big codebase.

We are already using php-cs-fixer but:

  • not run in our pipeline (it is not run in Display Builder either, so I don't know if we wnat that or not)
  • don't use the standard drupal8 ruleset
  • ignore tests files

Proposed resolution

  • Don't ignore test files anymore.
  • Use the drupal8 ruleset, without the 3 rules which are changing too much LOC, so which are breaking too much MR:
    • ordered_class_elements
    • native_function_invocation
    • blank_line_before_statement

Those rules will be added later, when the project will be less active.

If possible, add the tool in the pipeline.

Issue fork ui_patterns-3608929

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

pdureau created an issue. See original summary.

pdureau opened merge request !521

pdureau’s picture

pdureau commented
Assigned: pdureau » grimreaper
Status: Active » Needs review

Main changes:

  • from double quote to simple quote ✅
  • indentation & blank space normalization ✅
  • removal of redundant docblocks ✅
  • from "==" to "===" ✅
  • stricter in_array function ✅
  • remove Yoda conditions. ⚠️ Why? Is it not a good practice?
  • @coversNothing added ion some tests. ⚠️ Does that mean we need to manually add the missing @cover annotation/attributes ? in a follow-up ticket?

This review is not for merge but for discussing.

After running php-cs-fixer, some previously undetected pipeline fails appeared. I guess it is a good thing. Maybe it was hidden and need to be addressed:

  • line break between case instructions ❌ incompatible with our phpcs rules, so i run phpcbf again after. What can we do?
  • ❌ "Line exceeds 80 characters" in ComponentFormDisplay.php That's interesting because it show some annotations have not been converted to attributes. Is it something we do in the socpe of the current ticket?
  • ❌ "Possible useless method overriding detected". To investigate. there is an opportunity to remove useless code.
  • ❌ "Method should return *Interface but returns object.". To investigate. Maybe some missing typing.

I don't know how to add the php-cs-fixer check to the pipeline. I twas not done in Display Builder project, maybe because we need a dependency to drupol/phpcsfixer-configs-drupal.

pdureau’s picture

pdureau commented
Assigned: grimreaper » pdureau
Status: Needs review » Needs work
pdureau’s picture

pdureau commented
Assigned: pdureau » grimreaper
Status: Needs work » Needs review

Green pipeline :)

Let's not merge before 2.0.17 release.

Also, it would be nice that the main maintainers do a cleanup of https://git.drupalcode.org/project/ui_patterns/-/merge_requests/ before:

  • closing the merged ones
  • and maybe some opened one which are outdated or old and empty

So we can evaluate the mess :)

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » pdureau
Status: Needs review » Needs work

Hi,

I reviewed the MR.

I left some comments.

pdureau’s picture

pdureau commented
Assigned: pdureau » grimreaper
Status: Needs work » Needs review

  • grimreaper committed a1ee9d4e on 2.0.x authored by pdureau 
    style: #3608929 Adopt php-cs-fixer ruleset

By: pdureau
By: grimreaper
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Needs review » Fixed

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.

Status: Fixed » Closed (fixed)

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