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

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

liam morland created an issue. See original summary.

liam morland’s picture

Assigned: liam morland » Unassigned
Status: Active » Needs review
smustgrave’s picture

Think if we are going to do this scope should be expanded to other files, or even postponed on a potential rector rule.

quietone’s picture

This 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.

liam morland’s picture

I did this file because it caused the parent issue to need a @phpstan-ignore rule added.

smustgrave’s picture

Still Think this should either be folded into the issue in #5 or expanded to more of core

graber’s picture

One 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.

quietone’s picture

Except 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.

liam morland’s picture

Unless 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.

smustgrave’s picture

I'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.

graber’s picture

@quietone, @smustgrave, are you two willing to do the entire views module in a reasonable amount of time?

quietone’s picture

@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?

graber’s picture

Ok, 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.

smustgrave’s picture

Status: Needs review » Closed (duplicate)

Definitely think at minimum it can be rolled into that issue.

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.