#Coding standards

Think this should be re-open?

This has essentially been RTBC since comment #13 on 7th April. What can we do to get it merged, so save you having to keep rebasing?

Rebased. There were conflicts due to the update of Coder, which fixed a problem with phpcs:ignore lines such that those lines could be removed. Linting passing so restoring RTBC.

An easy rebase, so restoring RTBC.

@krystalcode (#106)

The way Git merges changes does not have much to do - if anything - with the sorting of the contents of the code. Conflicts most commonly happen when you edit the same lines.

The problem with git vs imports is not just about actual conflicts.
If there is no strict order, there can also be duplication:

commit 1:

use N\A
+ use N\NewClass;
use N\B
use N\C

commit 2:

use N\A
use N\B
use N\C
+ use N\NewClass;

merge 1 + 2:

use N\A
use N\NewClass;
use N\B
use N\C
use N\NewClass;

Let's say we merge this, we notice the problem, and we want to clean it up.
But we actually have two branches that do this clean-up:

cleanup branch 1:

use N\A
- use N\NewClass;
use N\B
use N\C
use N\NewClass;

cleanup branch 2:

use N\A
use N\NewClass;
use N\B
use N\C
- use N\NewClass;

on merge, both of them are gone, and the import will be missing:

use N\A
use N\B
use N\C

Often enough, this does not happen in actual merges, but in a rebase or cherry-pick.
E.g. commit 1 above might be from a feature branch, then commit 2 is from a different feature that is merged upstream. Then branch 1 gets rebased on upstream, and what appears as "merge 1 + 2" above instead occurs as a commit in the rebase.

Having a strict order avoids these problems.
When there is ambiguity, the merge or rebase will see that, and the developer will be forced to sort it out.

When I write "new MyClass", the autocomplete will propose a matching qualified class name (class with namespace), and insert the import at the top.

Any convention we introduce here has to be one that is supported by commonly used tools, and does not require new custom sorting specific to Drupal.

Agreed. I never type the use statements; they are automatically added.

And what would cause Core to not come first? Every contrib module import is already after Core, as its namespaces are lowercase.

https://git.drupalcode.org/project/aws/-/blob/2.0.x/src/Entity/Profile.p...

In other words, code outside the Drupal namespace, mostly packages that live in /vendor/.
In fact, some 3rd party package namespaces will come before "Drupal\...", others will come after.
All of this is totally ok, and is not any different from e.g. symfony packages, where imports are sorted like "use Composer\...", "use Psr\...", "use Symfony\...", "use Twig\...".

These imports are going to be mostly managed and read by tools like IDEs or phpcbf.
As a developer, I don't want to scroll up and look at the imports.
When I write "new MyClass", the autocomplete will propose a matching qualified class name (class with namespace), and insert the import at the top.

Any convention we introduce here has to be one that is supported by commonly used tools, and does not require new custom sorting specific to Drupal.

https://git.drupalcode.org/project/aws/-/blob/2.0.x/src/Entity/Profile.p...

And what would cause Core to not come first? Every contrib module import is already after Core, as its namespaces are lowercase.

@solideogloria each to its preference, that's why PHPCS rules are customizable. My objection is to make one or the other as a MUST in the standards.

Just to point out the following in your comment though.

It makes more sense for Core use statements to come first, because Core code is the foundation, and the modules build on top of that. Plus, a single list of alphabetically ordered statements is the easiest to sort through visually

The first sentence is exactly what I'm proposing i.e. logical grouping of import statements, whether core or modules come first that's secondary.

The second sentence is the opposite i.e. alphabetical sorting without logical grouping. Drupal/Core is not the first set of packages alphabetically, it just happens to be so in the examples that I listed.

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.

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.

RE #103, I definitely prefer what it looks like as-is compared to the groupings you showed. It makes more sense for Core use statements to come first, because Core code is the foundation, and the modules build on top of that. Plus, a single list of alphabetically ordered statements is the easiest to sort through visually (and programmatically).

Since there's been no follow up going to close out. But don't worry this can always be re-opened if needed for D11.

Thanks all