2.x cs imports

[nlemoine]

Issue

Not really an issue but Timber was using different kinds of imports. Sometimes use Class, sometimes \Class.

Solution

Let's keep it consistent and add some rules about imports.

Impact

None.

Considerations

I personally add a leading slash on native functions in my projects/libs: https://cs.symfony.com/doc/rules/function_notation/native_function_invocation.html

I would also add this rule in Timber if you're ok with this @gchtr and @jarednova.
This provides a tiny performance bump.

Many popular libraries apply that rule (one way or another):
https://github.com/Yoast/wordpress-seo/blob/23d1e56b67e21f6e2d34c9a78e1ee7c55bb43844/src/main.php#L56
https://github.com/doctrine/orm/blob/c5e4e41e0517887b7ce5ffd2d825f9c337bc5cd4/lib/Doctrine/ORM/Configuration.php#L52
https://github.com/symfony/asset/blob/a0ebf67f56f028217256d5f99438175430b3836c/UrlPackage.php#L48

[coveralls]

coverage: 88.676%. remained the same when pulling fc73105 on 2.x-cs-imports into 8721d3e on 2.x.

[gchtr]

This can be handled automatically by ECS? I like that! 👍

This does qualify as a coding standard change, right? Then we could add it to https://github.com/timber/timber/blob/2.x/.git-blame-ignore-revs as well.

[nlemoine]

This can be handled automatically by ECS?

Absolutely 🙂

This does qualify as a coding standard change, right?

Yes, just waited for approval before adding the commit to the ignore revs

[nlemoine]

This can be handled automatically by ECS?

Oh, is your question also targeting the native function invocation?

[gchtr]

This can be handled automatically by ECS?

Oh, is your question also targeting the native function invocation?

Not necessarily. My question was more like a rhetoric question. It was more like a remark, but formulated as a question, meaning: I didn’t know this could be handled by ECS!

But let’s answer that other question, too!

I personally add a leading slash on native functions in my projects/libs: cs.symfony.com/doc/rules/function_notation/native_function_invocation.html

I would also add this rule in Timber if you're ok with this @gchtr and @jarednova. This provides a tiny performance bump.

If this can too be automated and is a performance improvement, then let’s go for it! 👍

[nlemoine]

Any supported rule in PHP CS Fixer can be handled automatically: https://cs.symfony.com/doc/rules/index.html

Any fixable rule for PHPCS too.

[gchtr]

Hey @nlemoine

This looks good to me! I saw that you also added the slashes for the native functions 👍.

So we only have to ignore the coding style commits and then we’re ready to merge. Or is there something else you still want to change?

[nlemoine]

Or is there something else you still want to change?

Yes, I'd like to remove ECS from composer dependencies. I noticed a couple of times that coding standards are not always the same depending on lowest/highest and/or PHP version.

I think it would better to avoid adding tooling in composer.json but I have to think about alternatives (phive for example).

[gchtr]

Yes, I'd like to remove ECS from composer dependencies. I noticed a couple of times that coding standards are not always the same depending on lowest/highest and/or PHP version.

I think it would better to avoid adding tooling in composer.json but I have to think about alternatives (phive for example).

Good idea. Now that you mention it, I also realized there were some inconsistencies in the standards, but I couldn’t see why.

Should we use a separate issue/PR to tackle that problem and finish this this pull request for now?

[gchtr]

Would an approach as Laravel Pint does it work? It uses Laravel Zero under the hood, which uses Box to build a standalone PHAR.

[gchtr]

@nlemoine Could we run the checks only when we use Composer dependencies with the highest option?

[gchtr]

@nlemoine I fixed the biggest failures for the ECS checks in #2779 with an update, when the checks will run. Let’s merge this in and look for better solutions to remove ECS from Composer dependencies in a different issue or PR :).