Problem/Motivation
Some types are incorrectly documented in ViewExecutable.
Proposed resolution
Fix them.
Remaining tasks
Implement.
User interface changes
None.
API changes
None.
Data model changes
None.
Issue fork drupal-3585443
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
Comment #3
liam morlandComment #4
smustgrave commentedThink if we are going to do this scope should be expanded to other files, or even postponed on a potential rector rule.
Comment #5
quietone commentedThis is similar to #3585410: Cleanup ViewExecutable @var and type hints, use interfaces for pager and style plugins
Yes, the scope needs to be considered here, it is not feasible to have one issue for each file in core.
Comment #6
liam morlandI did this file because it caused the parent issue to need a
@phpstan-ignorerule added.Comment #7
smustgrave commentedStill Think this should either be folded into the issue in #5 or expanded to more of core
Comment #8
graber commentedOne file is a start. Some core issues take years, if we can have a s mall improvement that’ll serve as an example for the next ones - I’d say let’s go for it. Also context of one file is much smaller so easier to handle automated tests.
Having bigger MRs means more time to handle and that means more work for resolving conflicts etc.. Would be good if we could have an epic and child issues.
Comment #9
quietone commentedExcept there are thousands of files in Drupal core, it makes better use of reviewer and committer time to not have one issue per file to fix the same type of 'thing'. That is the intent of the Issue scope guidelines for Drupal core issues. It is why Coding standards problem and phpstan problems are fixed by concept, not by file.
Comment #10
liam morlandUnless this is going to be automated, the result may be that this never gets fixed because doing it all at once is too hard. Years ago I was working on an issue like this. I made the same fix in many files, but by the time anyone reviewed it, it would need to be re-rolled. So I would re-roll and, again, by the time anyone reviewed it, it would need to be re-rolled. Nothing got fixed.
The primary condition for getting something merged should be that it makes Drupal better, not perfect. This meets that criteria.
Comment #11
smustgrave commentedI'm agreeing with @quietone doing a single file doesn't seem right. Either we should do the whole module (or group of modules) but not single individual files.
So reiterating may be worth folding into the one issue.
Comment #12
graber commented@quietone, @smustgrave, are you two willing to do the entire views module in a reasonable amount of time?
Comment #13
quietone commented@graber, it would not be done by module either. Fixes are done by concept across all of core. For example, have a look at how #3116859: [meta] Fix Drupal.Array.Array.LongLineDeclaration coding standard was completed.
Is there an automated way to say, find all the instances where "|null" should be added?
Comment #14
graber commentedOk, that’s an easy one but check my issue for the same thing that added 2 interfaces and updated automated tests.. that couldn’t be done in an automated way.. it’s referenced from this issue and is a kind of duplicate really.
Comment #15
smustgrave commentedDefinitely think at minimum it can be rolled into that issue.