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

Add support for PHP-CS-Fixer #10992

Merged
merged 3 commits into from
Aug 13, 2016
Merged

Add support for PHP-CS-Fixer #10992

merged 3 commits into from
Aug 13, 2016

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Jul 1, 2016

Summary of Changes

Adds support for the PHP-CS-Fixer tool as a means to help programmatically review and correct code style issues in PHP code. This initial implementation only includes the non-third-party libraries directory code.

Testing Instructions

Review the package's documentation to ensure the selected fixers are in line with the project's code style rules and review the changes made (sorry it's a lot of files but it's either that or do a PR for a couple directories at a time) to ensure they are consistent with the project's code style rules.

Note some changes in here are more for consistency and behavior than a PHPCS enforcement such as changing return null; to return; or the trailing comma on multi-line arrays.

@photodude
Copy link
Contributor

So this mostly just fixes coding standards issues related to PSR-1 and PSR-2?

@mbabker
Copy link
Contributor Author

mbabker commented Jul 2, 2016

Without writing custom fixers pretty much. It can also do some stuff we do
as a practice but not enforced in the standard like variable or array
key/value alignment. Considering we're one of the few who have standards
vastly opposite of PSR-2 there's only so much that can be pulled from the
source repo.

On Friday, July 1, 2016, Walt Sorensen notifications@github.com wrote:

So this mostly just fixes coding standards issues related to PSR-1 and
PSR-2?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#10992 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAWfoYQ7wnkEWNMcnim0llBqFC-Xua3yks5qRdiqgaJpZM4JDhHt
.

@laoneo
Copy link
Member

laoneo commented Jul 3, 2016

Thumbs up for the auto code fixer. To safely test this PR, why not breaking it into smaller peaces?

@photodude
Copy link
Contributor

photodude commented Jul 3, 2016

@laoneo We have another set of auto code fixers with the in development update to PHPCS2. There is still some work to be done verifying the code style coverage is valid and testing/verifying the fixers.

@pritalpatel
Copy link

I have tested this item 🔴 unsuccessfully on f4653fb

While I was trying to apply patch using patch tester first it took long time to apply then suddenly getting white page. Enabled error reporting to development but still not showing any error and just showing white/blank page. There is nothing in view source as well.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10992.

@pritalpatel
Copy link

But when I have downloaded https://codeload.github.com/mbabker/joomla-cms/zip/phpcs-fixer and install joomla using it that works.
I have tested joomla backend using gherkin scenarios and found it working.
screen shot 2016-07-04 at 09 12 57

Note: I haven't tested as per given testing info.

Review the package's documentation to ensure the selected fixers are in line with the project's code style rules

but I have tested that after this patch joomla admin works normally or not.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10992.

@pritalpatel
Copy link

I have tested this item ✅ successfully on f4653fb


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10992.

@pritalpatel
Copy link

Also wants to mention that I haven't checked all the area of admin. I have checked Users, Content, Category, Access Group and levels.

@ghost
Copy link

ghost commented Jul 5, 2016

I have tested this item ✅ successfully on f4653fb

Checked all applied fixers with description at https://github.com/FriendsOfPhp/PHP-CS-Fixer
and compared with coding standards http://joomla.github.io/coding-standards/?coding-standards/chapters/php.md

Applied patch 2days ago. Joomla worked fine without any issues 'til now.

Code review of all files of this PR.

Tested .php_cs configuration with several 3rd-party libraries and custom extensions and FE /components/ several hours and added "wrong code styles". Success.

Couldn't find any break of Joomla Coding Standards produced by PHP-CS-Fixer. Just "nicer" and more consistent code than before.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10992.

@brianteeman
Copy link
Contributor

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10992.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 5, 2016
@roland-d roland-d added this to the Joomla 3.7.0 milestone Jul 16, 2016
@wilsonge
Copy link
Contributor

@mbabker Rebase this please

@mbabker
Copy link
Contributor Author

mbabker commented Aug 13, 2016

Done

@wilsonge wilsonge modified the milestones: Joomla 3.6.3, Joomla 3.7.0 Aug 13, 2016
@wilsonge wilsonge merged commit 979e5ef into joomla:staging Aug 13, 2016
@mbabker mbabker deleted the phpcs-fixer branch August 13, 2016 18:27
@brianteeman brianteeman removed the RTC This Pull Request is Ready To Commit label Aug 13, 2016
ggppdk pushed a commit to ggppdk/joomla-cms that referenced this pull request Aug 19, 2016
* Add dev dependency for php-cs-fixer

* Add php-cs-fixer config for libraries/cms folder

* Add all libraries files to php-cs-fixer config, ignore config file in build script
roland-d pushed a commit to roland-d/joomla-cms that referenced this pull request Sep 11, 2016
* Add dev dependency for php-cs-fixer

* Add php-cs-fixer config for libraries/cms folder

* Add all libraries files to php-cs-fixer config, ignore config file in build script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants