One of the changes for Coder 9 is that Squiz.Classes.ClassFileName replaces Drupal.Classes.ClassFileName. Unfortunately, it has a different behavior in that it does not allow two classes to be declared in a single file, which is done in many Drupal tests. That is resulting in a large MR in the parent issue.
Those changes are moved to this issue.
Replace Drupal.Classes.ClassFileName with Squiz.Classes.ClassFileName
Review
| Comment | File | Size | Author |
|---|
Show commandsStart 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:
changes, plain diff MR !15741
Comments
Comment #3
quietone commentedComment #4
borisson_The change in phpcs.xml.dist looks good and most of the changes I scanned through also look good, I don't understand why phpstan baseline file has changed?
Comment #5
smustgrave commentedfrom what I can tell the baseline changed due to some phpstan lines being removed
@phpstan-ignore testClass.missingAttribute.Group, testClass.missingAttribute.RunInSeparateProcessesfor example.Comment #6
borisson_Looks like that's it, let's make sure we don't need to do those changes if we can, then this looks good to go.
Comment #7
longwaveYeah, I think those phpstan-ignores are there by design, we never want that class to have the attributes?
Comment #8
quietone commentedI've restored the phpstan-ignore .
Comment #9
borisson_This looks good to me now.
Comment #10
longwaveTo me adding test fixtures inside the same file makes more sense than keeping them separate. If we introduce this here then we have to explicitly add an ignore for every single test fixture that is handled in this way - shouldn't we consider enabling this rule only for non-test files?
Comment #11
longwaveThe coding standards say "Best practices include having one class or interface or trait per file. That file should be named for the class, such that the file name for FooInterface would be FooInterface.php." - ie to me this is a best practice but not a hard requirement.
Maybe we should follow RFC 2119 for the coding standards - ie. use MUST and SHOULD for requirements vs recommendations.
Comment #12
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #13
borisson_Most test files only have one class, so it's only for a few of them that we need to add this rule, so if we can keep doing it like this, that makes more sense to me.
Comment #14
quietone commentedIt is roughly 171/3935 or 4% of files ending in 'Test.php'. So, yes, it isn't a lot of files. On the other hand, excluding test file is what we are doing now. Maybe it is best to keep to current practice while we update to Coder 9.
Comment #15
quietone commentedI should have set this to needs review.
I still think longwave is correct. Let's not do this, or at least, not now. I think we can explore if we want to change how we write some tests in the Coding Standards project.
Comment #16
smustgrave commentedCoder 9 just landed not sure if anything needs to be tweaked here.
Comment #17
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #18
quietone commentedYes, I think this is really working as designed.