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

Follow coding-standard PER-CS2.0 #4372

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Nov 18, 2024

Description (*)

We run coding-rule checks since ages, but we never followed a common standard, We had some PSR2/12-rules for PHPCS, but settings for php-cs-fixer where different.

@github-actions github-actions bot added environment Component: PayPal Relates to Mage_Paypal Component: Core Relates to Mage_Core Component: Catalog Relates to Mage_Catalog Template : admin Relates to admin template Component: Cms Relates to Mage_Cms Component: Reports Relates to Mage_Reports Component: CatalogInventory Relates to Mage_CatalogInventory Component: Checkout Relates to Mage_Checkout Component: AdminNotification Relates to Mage_AdminNotification Component: lib/Varien Relates to lib/Varien Component: Sales Relates to Mage_Sales Component: Usa Relates to Mage_Usa Component: Customer Relates to Mage_Customer Component: Eav Relates to Mage_Eav Template : rwd Relates to rwd template Component: lib/Mage Relates to lib/Mage Template : base Relates to base template Component: Adminhtml Relates to Mage_Adminhtml Component: ProductAlert Relates to Mage_ProductAlert Mage.php Relates to app/Mage.php Component: Sitemap Relates to Mage_Sitemap Component: Page Relates to Mage_Page Component: Api PageRelates to Mage_Api Component: Cron Relates to Mage_Cron Component: Captcha Relates to Mage_Captcha labels Nov 18, 2024
@github-actions github-actions bot added Component: Rss Relates to Mage_Rss Component: Paygate Relates to Mage_Paygate Component: lib/* Relates to lib/* composer Relates to composer.json errors Relates to error pages phpcs php-cs-fixer phpunit Component: lib/Magento Relates to lib/Magento labels Nov 18, 2024
@addison74
Copy link
Contributor

I wanted to check all the changes in the more than 3000 files. Although I have an extremely high-performance system, Chrome threw in the towel several times. The page already consumes 2.5 GB of RAM and is growing.

@sreichel
Copy link
Contributor Author

sreichel commented Nov 18, 2024

No need to check manually.

Its only about code styling.

  • adding comma to last array-element if its a multiline-list
  • shorten empty classes
  • adding space between (type)$valiable
  • ...

See

@sreichel
Copy link
Contributor Author

sreichel commented Nov 18, 2024

Chrome threw in the towel several times. The page already consumes 2.5 GB of RAM and is growing.

Sorry for offopic, but what are you working with?

I only have an Intel Xeon E3-1231 with 16GB RAM and everything runs smooth. Even in VirtualBox.

@sreichel sreichel requested a review from kiatng November 19, 2024 06:16
# Conflicts:
#	composer.json
#	composer.lock
@kiatng
Copy link
Contributor

kiatng commented Nov 19, 2024

Ref https://www.php.net/manual/en/functions.arguments.php, trailing comma only supports in PHP8.

I can understand trailing comma is to reduce version control diff noise but this PR actually add more noise. It's good for project under development, I don't see benefit applying here. Why is it good for OpenMage?

@sreichel
Copy link
Contributor Author

sreichel commented Nov 19, 2024

Ref https://www.php.net/manual/en/functions.arguments.php, trailing comma only supports in PHP8.

This is for function parameters only. The PR is about array items.

Why is it good for OpenMage?

We only had a couple of rules. Some from PSR2, some from PRS12. Not fish, not meat.

PSR-12 was accepted in 2019 and since then a number of changes have been made to PHP which have implications for coding style guidelines. Whilst PSR-12 is very comprehensive of PHP functionality that existed at the time of writing, new functionality is very open to interpretation. This PER, therefore, seeks to clarify the content of PSR-12 in a more modern context with new functionality available, make the errata to PSR-12 binding and further update it according to new data and language changes.

https://www.php-fig.org/per/coding-style/

@sreichel
Copy link
Contributor Author

I can understand trailing comma is to reduce version control diff noise

Trailing comma is for better programming workflow!

You are on the last element of an array ... you hit ctrl + d to duplicate this line - to add an entry. How often you had to go to previous line to add that comma?

@sreichel
Copy link
Contributor Author

sreichel commented Nov 19, 2024

But I tested here for PHP7.4, which throws error.

This is NOT part of the PR.

Trailing comma ...

Old ....

$a = [
    [],
    [] # no trailing comma
];

New ...

$a = [
    [],
    [], # trailing comma
];

Edit:

"PHP8-trailing-comma"

public function(
    $a,
    $b, #php8-only
)

Edit2:

All checks test for valid php7 syntax ;)

@sreichel
Copy link
Contributor Author

Thats not that problem ... in https://onlinephp.io/c/0b1cf the error is on line 2. The comma in test(), not that comma in explode().

@sreichel
Copy link
Contributor Author

It's this, I managed to screenshot before my browser crashed:

try https://patch-diff.githubusercontent.com/raw/OpenMage/magento-lts/pull/4372.diff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Admin Relates to Mage_Admin Component: Adminhtml Relates to Mage_Adminhtml Component: AdminNotification Relates to Mage_AdminNotification Component: Api PageRelates to Mage_Api Component: Api2 Relates to Mage_Api2 Component: Authorizenet Relates to Mage_Authorizenet Component: Bundle Relates to Mage_Bundle Component: Captcha Relates to Mage_Captcha Component: Catalog Relates to Mage_Catalog Component: CatalogIndex Relates to Mage_CatalogIndex Component: CatalogInventory Relates to Mage_CatalogInventory Component: CatalogRule Relates to Mage_CatalogRule Component: CatalogSearch Relates to Mage_CatalogSearch Component: Centinel Relates to Mage_Centinel Component: Checkout Relates to Mage_Checkout Component: Cms Relates to Mage_Cms Component: ConfigurableSwatches Relates to Mage_ConfigurableSwatches Component: Contacts Relates to Mage_Contacts Component: Core Relates to Mage_Core Component: Cron Relates to Mage_Cron Component: CurrencySymbol Relates to Mage_CurrencySymbol Component: Customer Relates to Mage_Customer Component: Dataflow Relates to Mage_Dataflow Component: Directory Relates to Mage_Directory Component: Downloadable Relates to Mage_Downloadable Component: Eav Relates to Mage_Eav Component: GoogleAnalytics Relates to Mage_GoogleAnalytics Component: ImportExport Relates to Mage_ImportExport Component: Index Relates to Mage_Index Component: Install Relates to Mage_Install Component: lib/Mage Relates to lib/Mage Component: lib/Magento Relates to lib/Magento Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Log Relates to Mage_Log Component: Media Relates to Mage_Media Component: Newsletter Relates to Mage_Newsletter Component: Oauth Relates to Mage_Oauth Component: Page Relates to Mage_Page Component: Paygate Relates to Mage_Paygate Component: Payment Relates to Mage_Payment Component: PayPal Relates to Mage_Paypal Component: ProductAlert Relates to Mage_ProductAlert Component: Rating Relates to Mage_Rating Component: Reports Relates to Mage_Reports Component: Review Relates to Mage_Review Component: Rss Relates to Mage_Rss Component: Rule Relates to Mage_Rule Component: Sales Relates to Mage_Sales Component: SalesRule Relates to Mage_SalesRule Component: Shipping Relates to Mage_Shipping Component: Sitemap Relates to Mage_Sitemap Component: Tag Relates to Mage_Tag Component: Tax Relates to Mage_Tax Component: Uploader Relates to Mage_Uploader Component: Usa Relates to Mage_Usa Component: Weee Relates to Mage_Weee Component: Widget Relates to Mage_Widget Component: Wishlist Relates to Mage_Wishlist composer Relates to composer.json documentation needed environment errors Relates to error pages Mage.php Relates to app/Mage.php php-cs-fixer phpcs phpunit shell Relates to shell scripts Template : admin Relates to admin template Template : base Relates to base template Template : rwd Relates to rwd template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants