Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #4274: Let lowercase_constants directive to be configurable. #4275

Open
wants to merge 11 commits into
base: master
from

Conversation

@drupol
Copy link
Contributor

commented Jan 21, 2019

Fix #4274

Depends on #4362

This issue is to have the lowercase_constants directive configurable. (Issue #3026)
The configurable option would let you choose in between 'upper' or 'lower' case syntax.

Here's an example on how it would be:

lowercase_constants:
   case: upper

or

lowercase_constants:
   case: lower

The default would be lower to be backward compatible.

Why this request ?
CMS's like Drupal 8 are using uppercase constants (TRUE, FALSE, NULL) and I would like to have a proper tool that follows the Drupal 8 "standard" in our custom code at European Commission.

The PHP version you are using ($ php -v):

=> PHP 7.2.14

PHP CS Fixer version you are using ($ php-cs-fixer -V):

=> PHP CS Fixer 2.14.1-DEV Sunrise by Fabien Potencier and Dariusz Ruminski

The command you use to run PHP CS Fixer:

=> Via grumphp

The configuration file you are using, if any:

        array_syntax:
            syntax: short
        blank_line_after_opening_tag: true
        blank_line_before_statement:
            statements:
                - break
                - continue
                - declare
                - return
                - throw
                - try
        include: true
        # Drupalism:
        class_attributes_separation: false
        declare_equal_normalize:
            space: single
        declare_strict_types: true
        is_null: true
        lowercase_static_reference: true
        # Drupalism: lowercase_constants (unable to configure it in php-cs-fixer. Provide a patch?)
        native_function_invocation: false
        # Drupalism:
        no_blank_lines_after_class_opening: false
        no_closing_tag: true
        no_extra_blank_lines:
            tokens:
                - extra
                # Drupalism: - curly_brace_block
                - parenthesis_brace_block
                - square_brace_block
                - return
                - throw
                - use
                - use_trait
        no_trailing_whitespace_in_comment: true
        no_unneeded_final_method: true
        single_blank_line_at_eof: true
        single_line_after_imports: true
        strict_comparison: true
        strict_param: true
        trailing_comma_in_multiline_array: true
        yoda_style:
            equal: false
            identical: false
            less_and_greater: false
            always_move_variable: false

If applicable, please provide minimum samples of PHP code (as plain text, not screenshots):

  • before running PHP CS Fixer (no changes):
if ($var === null) {
  // Do something
}
  • with unexpected changes applied when running PHP CS Fixer:
if ($var === null) {
  // Do something
}
  • with the changes you expected instead:
if ($var === NULL) {
  // Do something
}
@dmvdbrugge
Copy link
Contributor

left a comment

While I'm 👎 for the idea given the reasons in the previously closed issue (but let's not discuss that here), I did do a review for it. Most of it boils down to opinion though, please let the maintainers weigh in as well.

An additional note is this should target master as this is a feature and not a bugfix.

src/Fixer/Casing/LowercaseConstantsFixer.php Outdated Show resolved Hide resolved
src/Fixer/Casing/LowercaseConstantsFixer.php Outdated Show resolved Hide resolved
src/Fixer/Casing/LowercaseConstantsFixer.php Outdated Show resolved Hide resolved
src/Fixer/Casing/LowercaseConstantsFixer.php Outdated Show resolved Hide resolved
src/Fixer/Casing/LowercaseConstantsFixer.php Outdated Show resolved Hide resolved

@drupol drupol force-pushed the drupol:4274-lowercase-constants-configurable branch 2 times, most recently from 52b2036 to 848a53c Jan 21, 2019

@drupol drupol changed the base branch from 2.14 to master Jan 21, 2019

@drupol

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

Branch has been rebased against master, as requested by @dmvdbrugge in first review comment.

@dmvdbrugge

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

The reason why your builds keep failing by the way is laid out in the CONTRIBUTING.md

Regenerate README: php php-cs-fixer readme > README.rst. Do not modify README.rst manually!

@dmvdbrugge
Copy link
Contributor

left a comment

Even though I would vote against merging, the PR itself gets my approval.

@SpacePossum

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

Hi and thanks for opening this PR.

Before I review the PR in detail, I'm wondering. The suggestion to uppercase the keywords is not in line with PSR2 , however this is part of the Drupal standard? Do you know why Drupal prefers this style or if this is legacy they might want to move away from?

@drupol

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2019

Hello,

No I don't know why using uppercase for keywords was chosen in Drupal.
I'm pretty sure that it won't change in Drupal 7 and 9 and I don't know either if they are going to move away from it in Drupal 9.
However, I really wish they do.

That said, having this configurable in PHP-CS-Fixer shouldn't be a bad thing, I think.

@SpacePossum

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

I'm +1 as this is a standard of a bigger community (Drupal) even if against PSR, however I know of and respect the opinions of people who feel different about this change.

(but the PR itself needs some tweaking still)

@dmvdbrugge

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

Final nitpicking remark from me: I find "syntax" a strange choice for configuration option, "case" or "casing" would be more clear and intuitive.

@drupol

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

Ok guys, I will fix this, it will be ready for tomorrow.

@SpacePossum

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

(maybe case would be best, as we already used that as config option else where, but casing we did not)

@drupol

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

@dmvdbrugge @SpacePossum Ready for review again.

@SpacePossum

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

please regenerate the readme with php php-cs-fixer readme > README.rst :)

@drupol drupol force-pushed the drupol:4274-lowercase-constants-configurable branch from 08e4802 to 30a8bab Feb 20, 2019

@drupol

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

All good @SpacePossum

@drupol drupol force-pushed the drupol:4274-lowercase-constants-configurable branch from 6042e7f to dff2bef Mar 3, 2019

src/Fixer/Casing/LowercaseConstantsFixer.php Outdated Show resolved Hide resolved
src/Tokenizer/Analyzer/ClassAnalyzer.php Outdated Show resolved Hide resolved
src/Tokenizer/Analyzer/ClassAnalyzer.php Outdated Show resolved Hide resolved
src/Tokenizer/Analyzer/ClassAnalyzer.php Outdated Show resolved Hide resolved
src/Tokenizer/Analyzer/ClassAnalyzer.php Outdated Show resolved Hide resolved
src/Tokenizer/Analyzer/ClassAnalyzer.php Outdated Show resolved Hide resolved
src/Tokenizer/Analyzer/ClassAnalyzer.php Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
src/Tokenizer/Analyzer/ClassAnalyzer.php Outdated Show resolved Hide resolved

@drupol drupol force-pushed the drupol:4274-lowercase-constants-configurable branch from a214518 to 0db09de Jul 23, 2019

@drupol

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@keradus @dmvdbrugge @SpacePossum Any of you guys to do a new review ?

@drupol drupol force-pushed the drupol:4274-lowercase-constants-configurable branch from 0db09de to 326f47c Jul 27, 2019

@drupol drupol force-pushed the drupol:4274-lowercase-constants-configurable branch 2 times, most recently from 0fd923d to fe44675 Aug 5, 2019

@drupol drupol force-pushed the drupol:4274-lowercase-constants-configurable branch 3 times, most recently from 863e695 to dc1e98d Aug 7, 2019

src/Fixer/Casing/ConstantCaseFixer.php Outdated Show resolved Hide resolved
src/Fixer/Casing/ConstantCaseFixer.php Outdated Show resolved Hide resolved
$this->fixFunction = 'strtolower';
}
if ('upper' === $this->configuration['case']) {

This comment has been minimized.

Copy link
@kubawerlos

kubawerlos Aug 12, 2019

Contributor

else instead of this if?

This comment has been minimized.

Copy link
@drupol

drupol Aug 14, 2019

Author Contributor

I'm always trying to avoid else|elseif conditions to have a easier code to read and to usually reduce the cyclomatic complexity. Is this really necessary ?

src/Fixer/Casing/ConstantCaseFixer.php Outdated Show resolved Hide resolved

@drupol drupol force-pushed the drupol:4274-lowercase-constants-configurable branch from d8b8fa4 to 2de7488 Sep 10, 2019

@drupol drupol force-pushed the drupol:4274-lowercase-constants-configurable branch from f98e682 to fcdce90 Sep 10, 2019

@drupol drupol force-pushed the drupol:4274-lowercase-constants-configurable branch from fcdce90 to 7afca68 Sep 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.