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

Code style (endif endforeach endfor and more) #1035

Merged
merged 13 commits into from
Apr 29, 2021

Conversation

luigifab
Copy link
Contributor

@luigifab luigifab commented Jun 10, 2020

Description

This PR updates code style of PHP code (#1027).

For express review, perhaps you can:

  • git clone 1.9.4.x in base/
  • git clone endif-endwhile-endforeach in changes/

Then, open all modified files of base/ directory in your text editor, search and replace (case sensitive) in all files (from bottom to top):

<?php endfor; ?>      by  <?php endfor ?>
<?php endfor;?>       by  <?php endfor ?>
<?php else :?>        by  <?php else: ?>
<?php else : ?>       by  <?php else: ?>
<?php else:?>         by  <?php else: ?>
<?php endforeach; ?>  by  <?php endforeach ?>
<?php endforeach;?>   by  <?php endforeach ?>
<?php endforeach?>    by  <?php endforeach ?>
<?php endif; //       by  <?php endif //
<?php //endif; ?>     by  <?php //endif ?>
<?php endif; ?>       by  <?php endif ?>
<?php endif;?>        by  <?php endif ?>
<?php endif?>         by  <?php endif ?>

Then, diff -r -U0 base/ changes/

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@sreichel sreichel added the Cleanup: Code style Related to simple CS fixes. label Jun 10, 2020
Copy link
Contributor

@kkrieger85 kkrieger85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After 300 files I'd say: LGTM

Few whitespace only changes, but looks good.

Need to discuss: We should change the review workflow for this kind of PR. Nobody want's to review 800 files manually and it's very time consuming (for me: 2 1/2hours for 300 files => estimated 6-7hours for all files)

@kkrieger85 kkrieger85 added the review needed Problem should be verified label Jun 12, 2020
@colinmollenhour
Copy link
Member

Need to discuss: We should change the review workflow for this kind of PR. Nobody want's to review 800 files manually and it's very time consuming (for me: 2 1/2hours for 300 files => estimated 6-7hours for all files)

@luigifab If you applied the change with a command line tool perhaps you could supply the command used so we could easily reproduce it?

@luigifab
Copy link
Contributor Author

Sorry, no command used, I search all files, open all files, search/replace string with my text editor, save and commit.

Copy link
Contributor

@kkrieger85 kkrieger85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

509/805 files reviewed

still LGTM

kkrieger85
kkrieger85 previously approved these changes Jun 24, 2020
Copy link
Contributor

@kkrieger85 kkrieger85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally: LGTW

thx @luigifab for this effort

colinmollenhour
colinmollenhour previously approved these changes Jul 6, 2020
@colinmollenhour
Copy link
Member

@luigifab Sorry about the conflicts, can you merge the latest to resolve?

Copy link
Contributor Author

@luigifab luigifab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, my bad, I rebased the branch from 1.9.4.x instead of merging it :/
I don't understand why there are xmlconnect files in Files changed tab.

@luigifab luigifab changed the title Endif endwhile endforeach endif endwhile endforeach endfor Jul 7, 2020
@luigifab luigifab marked this pull request as draft July 8, 2020 18:04
@luigifab luigifab marked this pull request as ready for review July 8, 2020 19:04
@luigifab luigifab changed the title endif endwhile endforeach endfor Code style (endif endforeach endfor and more) Jul 11, 2020
@luigifab luigifab force-pushed the endif-endwhile-endforeach branch 2 times, most recently from c4462b5 to 6d235e7 Compare July 21, 2020 18:56
@kkrieger85
Copy link
Contributor

@luigifab Please stop to add more commits here ;)

I think it would be better to create separate PR for different Code Style changes.
That would be improve the code review process.

@kkrieger85
Copy link
Contributor

See discussion about PHPCSFIXER in #947

@sreichel sreichel linked an issue Jul 23, 2020 that may be closed by this pull request
@github-actions github-actions bot added Component: Tag Relates to Mage_Tag Component: Tax Relates to Mage_Tax Component: Weee Relates to Mage_Weee Component: Widget Relates to Mage_Widget Component: Wishlist Relates to Mage_Wishlist errors Relates to error pages Template : admin Relates to admin template Template : base Relates to base template Template : default Relates to base template Template : install Relates to install template Template : rwd Relates to rwd template and removed Component: Admin Relates to Mage_Admin Mage.php Relates to app/Mage.php Component: Cm/RedisSession Relates to Cm_RedisSession labels Feb 21, 2021
Copy link
Contributor Author

@luigifab luigifab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, so sorry, I rebased and repushed only endxyz changes to get a diff.

@luigifab
Copy link
Contributor Author

luigifab commented Mar 21, 2021

I try to apply the diff but no... so I made a script to do the job:

<?php

$todo = [
	'<?php endfor; ?>'     => '<?php endfor ?>',
	'<?php endfor;?>'      => '<?php endfor ?>',
	'<?php else :?>'       => '<?php else: ?>',
	'<?php else : ?>'      => '<?php else: ?>',
	'<?php else:?>'        => '<?php else: ?>',
	'<?php endforeach; ?>' => '<?php endforeach ?>',
	'<?php endforeach;?>'  => '<?php endforeach ?>',
	'<?php endforeach?>'   => '<?php endforeach ?>',
	'<?php endif; //'      => '<?php endif //',
	'<?php //endif; ?>'    => '<?php //endif ?>',
	'<?php endif; ?>'      => '<?php endif ?>',
	'<?php endif;?>'       => '<?php endif ?>',
	'<?php endif?>'        => '<?php endif ?>',
];

foreach ($todo as $search => $replace) {

	echo $search,"\n";

	$files = [];
	exec('grep -Hlrn "'.$search.'" app/', $files);

	foreach ($files as $file) {
		echo '  ',$file,"\n";
		file_put_contents($file, str_replace($search, $replace, file_get_contents($file)));
		// system(git add app/ ; git commit -m "")
	}
}

@fballiano
Copy link
Contributor

@luigifab loving this PR as I really didn't like the old syntax :-)

Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the PHP script on the same base commit and the result was slightly different but no material differences so I approve so this can get merged sooner than later. Attached is the diff that was generated.
diff.patch.txt

@colinmollenhour colinmollenhour merged commit 5a0b657 into OpenMage:1.9.4.x Apr 29, 2021
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
6 runs  +4  4 ✔️ +2  0 💤 ±0  2 ❌ +2 

Results for commit 5a0b657. ± Comparison against base commit e4c2c4c.

@ADDISON74
Copy link
Contributor

Once this will be released with a new OM version I recommend a revision of the code looking for some parts unchanged. I will do that too.

A script (e.g. PHP, bash) will be useful to change any custom theme by everyone interested. I guess the one provided by @luigifab is just fine.

@ADDISON74
Copy link
Contributor

What is your opinion about <?php break; ?> ?

Should it be <?php break ?>?

@fballiano
Copy link
Contributor

@ADDISON74 you've a great eye, I added "break" as a new PR #1577

@luigifab luigifab deleted the endif-endwhile-endforeach branch April 30, 2021 06:54
Flyingmana pushed a commit that referenced this pull request Apr 30, 2021
Co-authored-by: Fabrizio Balliano <fabrizio@fabrizioballiano.com>
@luigifab luigifab mentioned this pull request Jun 25, 2021
3 tasks
randallelliott714 added a commit to randallelliott714/magento that referenced this pull request Oct 18, 2022
Co-authored-by: Fabrizio Balliano <fabrizio@fabrizioballiano.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup: Code style Related to simple CS fixes. Component: Adminhtml Relates to Mage_Adminhtml 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: CatalogSearch Relates to Mage_CatalogSearch Component: Centinel Relates to Mage_Centinel Component: Checkout Relates to Mage_Checkout Component: Cms Relates to Mage_Cms Component: CurrencySymbol Relates to Mage_CurrencySymbol Component: Customer Relates to Mage_Customer Component: Directory Relates to Mage_Directory Component: Downloadable Relates to Mage_Downloadable Component: Eav Relates to Mage_Eav Component: Giftmessage Relates to Mage_Giftmessage Component: GoogleAnalytics Relates to Mage_GoogleAnalytics Component: Index Relates to Mage_Index Component: Newsletter Relates to Mage_Newsletter Component: Oauth Relates to Mage_Oauth Component: Page Relates to Mage_Page Component: PageCache Relates to Mage_PageCache Component: Paygate Relates to Mage_Paygate Component: Payment Relates to Mage_Payment Component: PayPal Relates to Mage_Paypal Component: Persistant Relates to Mage_Persistant Component: Poll Relates to Mage_Poll 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: Sales Relates to Mage_Sales Component: Sendfriend Relates to Mage_Sendfriend Component: Shipping Relates to Mage_Shipping Component: Tag Relates to Mage_Tag Component: Tax Relates to Mage_Tax Component: Weee Relates to Mage_Weee Component: Widget Relates to Mage_Widget Component: Wishlist Relates to Mage_Wishlist errors Relates to error pages review needed Problem should be verified Template : admin Relates to admin template Template : base Relates to base template Template : default Relates to base template Template : install Relates to install template Template : rwd Relates to rwd template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion about endif/endforeach
6 participants