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

Upgrade to PSR12 coding standard #20121

Merged
merged 11 commits into from
Mar 19, 2024

Conversation

razvanphp
Copy link
Contributor

@razvanphp razvanphp commented Mar 2, 2024

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Fixed issues yiisoft/yii2-coding-standards#42

Copy link

what-the-diff bot commented Mar 2, 2024

PR Summary

  • Added New Code Quality Workflow
    A new file, 'lint.yaml', has been added to perform automatic checks for code quality in the project.

  • New Dependencies in Composer
    Two new dependencies added to project's composer.json file to support and maintain code quality.

  • Code Clean-up Across Multiple Files
    Reformatted coding sections for improved readability and code understanding across numerous files including 'BaseYii.php', 'yii.js', 'yii.validation.js', etc.

  • Database Query Optimizations
    Changes made to optimize and improve the performance of database queries in various files like 'QueryBuilder.php', 'Schema.php', 'Command.php' and more. These changes cater to methods like 'queryScalar', 'normalizeSelect', 'buildOrderByAndLimit' etc. in different files.

  • Update in Code Annotations
    Added new annotations to 'Yii.php' for proper handling of SideEffects and MissingNamespace.

  • Modification in BaseFormatConverter
    In 'BaseFormatConverter.php', changes made for different date formats representation in 'convertDatePhpToIcu' method to improve its readability and usability

  • Two New Methods Added
    In 'BaseStringHelper.php', two new methods 'mb_ucfirst' and 'mb_ucwords' have been added to support string manipulations better.

  • Additional Checks Added to Multiple Methods
    Various methods like 'loadFallbackMessages', 'getProfiling', 'prepareName', 'checkTargetRelationExistence' and more have been updated with additional checks for improved performance and features.

  • Changes for Better Readability
    Multiple files like '_addColumns.php', '_createTable.php' and '_foreignTables.php' have been updated with better formatting to enhance readability.

  • Updates in Code Syntax
    In 'framework/web/DbSession.php', anonymous function syntax is now being used for better code structure and readability.

  • Refactoring in 'Request.php' and 'Session.php'
    Several changes were made in 'framework/web/Request.php' and 'framework/web/Session.php' to improve code understandability, eliminate redundancy and optimize execution.

  • Pattern Handling in 'UrlRule.php'
    Updated conditions for handling slashes in patterns in 'translatePattern' method in 'UrlRule.php'.

  • Special Cases Handling in 'MaskedInput.php'
    Special cases for certain options are now better handled in 'initClientOptions' method in 'MaskedInput.php' file.

Copy link

codecov bot commented Mar 2, 2024

Codecov Report

Attention: Patch coverage is 63.75000% with 58 lines in your changes are missing coverage. Please review.

Project coverage is 48.01%. Comparing base (b0aa6ab) to head (8620cbc).

Files Patch % Lines
framework/helpers/BaseFormatConverter.php 2.77% 35 Missing ⚠️
framework/console/ErrorHandler.php 0.00% 4 Missing ⚠️
framework/db/pgsql/QueryBuilder.php 42.85% 4 Missing ⚠️
framework/behaviors/AttributesBehavior.php 50.00% 2 Missing ⚠️
framework/db/cubrid/Schema.php 0.00% 2 Missing ⚠️
framework/grid/DataColumn.php 60.00% 2 Missing ⚠️
framework/i18n/Formatter.php 50.00% 2 Missing ⚠️
framework/base/Security.php 50.00% 1 Missing ⚠️
framework/behaviors/AttributeBehavior.php 50.00% 1 Missing ⚠️
...ramework/console/controllers/MigrateController.php 0.00% 1 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #20121      +/-   ##
==========================================
- Coverage   48.02%   48.01%   -0.02%     
==========================================
  Files         445      445              
  Lines       43892    43909      +17     
==========================================
+ Hits        21080    21083       +3     
- Misses      22812    22826      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

framework/web/Request.php Outdated Show resolved Hide resolved
@razvanphp
Copy link
Contributor Author

razvanphp commented Mar 2, 2024

Funny thing is... because of PSR12.Files.FileHeader.SpacingAfterBlock we need to change every file to add a new line before the copyright docblock....

framework/console/Controller.php Outdated Show resolved Hide resolved
framework/console/controllers/MessageController.php Outdated Show resolved Hide resolved
framework/console/controllers/MessageController.php Outdated Show resolved Hide resolved
framework/db/BaseActiveRecord.php Outdated Show resolved Hide resolved
framework/web/Request.php Outdated Show resolved Hide resolved
framework/web/Request.php Outdated Show resolved Hide resolved
framework/db/BaseActiveRecord.php Outdated Show resolved Hide resolved
framework/views/_createTable.php Outdated Show resolved Hide resolved
@razvanphp razvanphp force-pushed the upgrade-to-psr12-coding-standard branch from 53e5b29 to 9170626 Compare March 3, 2024 18:29
framework/grid/Column.php Show resolved Hide resolved
framework/helpers/BaseFormatConverter.php Outdated Show resolved Hide resolved
@samdark
Copy link
Member

samdark commented Mar 4, 2024

Overall it looks good except what's mentioned in comments. Let's fix it and merge.

@razvanphp
Copy link
Contributor Author

razvanphp commented Mar 14, 2024

@rob006 do you think we should remove the cs folder and friendsofphp/php-cs-fixer dependancy at this point?
ref: ba0ab40 Travis is also gone...

@rob006
Copy link
Contributor

rob006 commented Mar 14, 2024

@razvanphp Yes, it was never finished and AFAIK never worked as part of CI.

@razvanphp
Copy link
Contributor Author

@samdark this is ready for merge FMPOV, but we need to bump the coding standard repo to use PSR12, then all the tests will pass.

BTW: why is this PHP 5.4 test failing now? seem that indeed array_column function was added only in PHP 5.5, so that's why it fails in 5.4, but we did not touch that file....

@samdark
Copy link
Member

samdark commented Mar 15, 2024

Done. Still errors.

@razvanphp
Copy link
Contributor Author

Done. Still errors.

but we need a major tag to bump the coding standards in composer.json here

@samdark
Copy link
Member

samdark commented Mar 15, 2024

PHP 5.4 error is very weird cause it doesn't error in master branch and function call is there.

@samdark
Copy link
Member

samdark commented Mar 15, 2024

Tagged 3.0.0.

@rob006
Copy link
Contributor

rob006 commented Mar 15, 2024

PHP 5.4 error is very weird cause it doesn't error in master branch and function call is there.

It could be provided by polyfill required by php-cs-fixer. Now that php-cs-fixer is gone, all its dependencies are also unavailable.

@razvanphp
Copy link
Contributor Author

Good catch @rob006, fixed the test in b2585a6

Now we have to decide what we do with: Squiz.NamingConventions.ValidVariableName.PrivateNoUnderscore

The framework code initially decided to use underscore for private vars, even tho PSR2 had this rule (which is now included in PSR12 too). Should we goo all-in without underscores and adhere to PSR12 fully or add underscore for missing ones?

@rob006
Copy link
Contributor

rob006 commented Mar 15, 2024

AFAIK underscore in Yii is not to indicate visibility, bot to avoid conflicts with virtual attributes provided by getters and setters. So sometimes you use it, sometimes not, I don't think there is a rule smart enough to cover this.

@samdark
Copy link
Member

samdark commented Mar 16, 2024

Underscores are to be kept. This is a technical thing, not just style.

@razvanphp
Copy link
Contributor Author

All checks have passed. Please see my last 2 commits.

@razvanphp razvanphp changed the title WIP: Upgrade to PSR12 coding standard Upgrade to PSR12 coding standard Mar 18, 2024
@samdark samdark merged commit e2a1670 into yiisoft:master Mar 19, 2024
68 checks passed
@samdark
Copy link
Member

samdark commented Mar 19, 2024

Merged. Thank you for working on it.

@razvanphp razvanphp deleted the upgrade-to-psr12-coding-standard branch March 19, 2024 17:44
skepticspriggan pushed a commit to skepticspriggan/yii2 that referenced this pull request Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants