Fix return type

Aditi Saraf → made their first commit to this issue’s fork.

HI @quietone , could you please mention here what need to change in return type . I have checked
1 core/lib/Drupal/Component/Plugin/PluginManagerBase.php
2 core/lib/Drupal/Core/Database/Connection.php
3 core/lib/Drupal/Core/Database/Schema.php files and i didn't get any return type value need to change .

i have changed the return type . Please review

@Aditi Saraf, thanks for taking an interest in this coding standards issue. I see you are relatively new to Drupal, so Welcome! I see that you made an fork and then asked me a question less that a day ago. If you need feedback, I recommend waiting at least several days for another developer to respond. Just to let you know that Drupal is transitioning to GitLab so it can be confusing but, in general, once an issue starts, or is established with either an MR or patch workflow it should stay that way. Otherwise it is just confusing for reviewers and committers. And yes, there are always exceptions but someone with more experience will provide guidance on an issue.

To answer your question, you need to look at the parent issue and understand how to work with phpcs. For this issue that would be to remove the sniff, Drupal.Commenting.FunctionComment.InvalidNoReturn, from the Drupal phpcs configuration file, phpcs.xml. Once that is done then you need to run phpcs on core. I find it easiest to run the core script, /core/scripts/dev/commit-code-check.sh. The output from that will list the files and the lines where errors occur. Any file with an error and that is listed in the issue summary needs to be fixed in this issue. Not all the errors will be fixed here, just the ones for the files listed in the summary.

Now, looking at the patches uploaded they seem too large for that changes needed here. Taking a closer look. Ah, what I see are many, many out of scope changes. Drupal has clear guidelines what is changed in an issue → so that the set of changes is (usually) fixing one 'thing' and one 'thing' only. This is considered good practice and helps reviewers, coders and also in the case if an issue needs to be reverted.

If you are going to use a patch workflow then it is good practice to upload an interdiff when making any change. There are instructions → .

Cheers,

@quietone thank you for your feedback .

Reviewed the changes and updates for Null returns make sense, didn't break anything.

LGTM.

This is a bit weird, because native return types don't allow for void to be used in a union type. It seems acceptable in the @return phpdoc annotation, but when we eventually introduce native return types for these we'll need to return something.

In fact, why do we have methods that just throw exceptions? Maybe they should belong to an interface instead and the expected return type can be documented there. One of the methods has a todo suggesting it could be moved to an abstract method. I think this needs further investigation.

@mstrelan, which item has the @todo?

It's in the existing docblock for createTableSql

How about we ignore the challenging ones for now so the sniff can be enabled?

That sounds like a good idea, there is still one void union type that needs updating.

Thanks.

This looks good now

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

Rebased and there were no conflicts, so restoring RTBC

This one needs a reroll

Rebased and regenerated the baseline for 4c45e111

Rebase seems good.

larowlan → closed merge request !9764

Committed to 11.x - thanks!

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