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

Init rector config #6048

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from
Draft

Init rector config #6048

wants to merge 11 commits into from

Conversation

yurabakhtin
Copy link
Contributor

@yurabakhtin yurabakhtin commented Jan 17, 2023

What kind of change does this PR introduce? (check at least one)

  • Feature
  • Code style update
  • Refactor

The PR fulfills these requirements:

  • It's submitted to the develop branch, not the master branch if no hotfix
  • All tests are passing
  • New/updated tests are included
  • Changelog was modified

Other information:
https://github.com/humhub/team_tasks/issues/271

@what-the-diff
Copy link

what-the-diff bot commented Jan 17, 2023

  • Added rector/rector to composer.json
  • Created a new file called rector.php in the root directory of humhub with some configuration for Rector

rector.php Outdated Show resolved Hide resolved
@@ -64,7 +64,7 @@ private function process(string $args = ''): int
}

$commands = [
'cd ' . Yii::getAlias('@webroot'),
'cd ' . Yii::getAlias('@config'),
Copy link
Contributor

Choose a reason for hiding this comment

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

@yurabakhtin I like the fact that rector.php is no longer in the root directory.

Maybe we can even move it to /protected/humhub/config? Because /protected/config/ is more for custom configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luke- Done in commit c6a9756.

@luke- luke- added this to the v1.15 milestone Mar 28, 2023
@luke- luke- modified the milestones: v1.15, v1.16 Jun 29, 2023
@luke- luke- mentioned this pull request Jul 25, 2023
4 tasks
@martin-rueegg
Copy link
Contributor

Awesome imitative, thank you! I've always tried to apply PHPCS&fixer. But there is hardly any clean file so far! :-)

Does it make sense to make the following issue public/move it to this repo?
https://github.com/humhub/team_tasks/issues/271

@luke-
Copy link
Contributor

luke- commented Jul 27, 2023

@martin-rueegg It is not possible to publish private issues. But we can also discuss here. Currently, we had to put this issue on hold due to other work. But I hope that after the v.15 and other releases there will be more time for it...

@martin-rueegg
Copy link
Contributor

I see. No problem.

Would it be an option to create a little bash script that runs a find over the source code tree and applies the current PHPCS Beautifier on all scripts? That would make it easier for now to adhere to at least some coding standards without changing any code (just layout).

I'm using PhpStorm and have setup to run PhpCS automatically. For new files this is great. But every time I touch an existing file, it always finds some things that do not adhere to the standard.

Having one script to run to fix all layout issues would be something that could be done by contributors or even added as a git hook. Could be a short term fix before rector comes into service?

@luke-
Copy link
Contributor

luke- commented Jul 28, 2023

Yes, would be cool to have some beautifier tool configuration in the root. Which can be executed in the project folder or subfolders. Ideally with some GitHub Action support. But we really need to ensure to not break something, when touching so many files.

I hope with Rector we can mainly optimizing/rewriting code (e.g. new PHP versions/deprecations) or add support for HumHub refactorings in modules.

@martin-rueegg
Copy link
Contributor

Actually, such a configuration already exits. It just does not seem to be enforced or even manually applied.

Find errors with:

$HUMHUB_VENDOR_BIN/phpcs --standard=$HUMHUB_PATH/phpcs.xml.dist $HUMHUB_PATH/protected/humhub/

Fix fixable errors with:

$HUMHUB_VENDOR_BIN/phpcbf --standard=$HUMHUB_PATH/phpcs.xml.dist $HUMHUB_PATH/protected/humhub/

Of course, the scripts can be configured with exclusions and stuff. But It would already help in getting rid of formatting issues. And I have not experienced that the automatically fixed errors would change the functioning of the code in any way.

@luke-
Copy link
Contributor

luke- commented Aug 2, 2023

@martin-rueegg FYI, I just did some tests in the admin core module with the present PHP_CodeSniffer configuration vs the/my standard PHPStorm configuration.

Commits:

  1. Initial PHPStorm Auto Code Format: 0db628e
  2. PHP_CodeSniffer Auto Fixes 9629f39
  3. PHPStorm Code Auto Code Format: 21218db
  4. PHP_CodeSniffer Again f271511

Somehow we need to align the PHP_CodeSniffer to the PHPStorm format configuration.

@martin-rueegg
Copy link
Contributor

Hey @luke- that looks pretty cool. So basically, the two are not congruent in terms of indentation only. On roughly 22 lines of the entire codebase.

I was not aware of a PhpStorm code style configuration. In fact, I was asking about in in this comment and since it was left without response, I thought there might be only the PHP_CodeSniffer configuration which made me turn off the codestyle in PhpStorm and only use the PHP PHP_CodeSniffer:

image
image

I've also set to respect .editorconfig (which should probably also be set to the same settings):
image

Would you be able to share your .idea/codeStyles/ content (and any other significant part of the PhpStorm config)?

(Just for reference: [How to format complete project source code in phpstorm?](https://stackoverflow.com/questions/33697915/how-to-format-complete-project-source-code-in-phpstorm). Also, reformatting can happen before-commit.)

@luke-
Copy link
Contributor

luke- commented Aug 3, 2023

@martin-rueegg I didn't made any changes to my PHPStorm code style configuration. Only few settings are loaded from the file in Git .editorConfig. However, these are probably also defaults in PS anyway. https://github.com/humhub/humhub/blob/master/.editorconfig

Probably, in cases of conflict, it would be best to find a solution that is acceptable to both tools.

Example:

PHPCS:

<div>
    <?= $form->field($model, 'dateInputDisplayFormat')->dropDownList([
          // ...
    ]);
?>
</div>

PHPStorm:

<div>
    <?= $form->field($model, 'dateInputDisplayFormat')->dropDownList([
          // ...
    ]);
    ?>
</div>

PHPStorm & PHPCS:

<div>
    <?= $form->field($model, 'dateInputDisplayFormat')->dropDownList([
          // ...
    ]); ?>
</div>

I did not know that it is also possible to format the code via PHPStorm using PHPCS.

Theoretically, we could also just require this setting from the developers and enforce this via GitHub Action.

Of course, I would prefer if both tools PHPStorm & PHPCS produce the identical output. Then you wouldn't be limited to one tool.

Do you know any other tools for automatic PHP formatting?

@martin-rueegg
Copy link
Contributor

martin-rueegg commented Aug 4, 2023

@luke-

@martin-rueegg I didn't made any changes to my PHPStorm code style configuration.

Ok, could you please share the configuration anyway, just to make sure we're having the same starting point. Could you maybe export your config (in Settings / Editor / Code Style / PHP - using the cog wheel next to the Scheme selection)
a) as EditorConfig file to the .editorconfig in your branch, and
b) as PhpStorm XML to something like .PhpStorm.CodeStyle.xml?

Probably, in cases of conflict, it would be best to find a solution that is acceptable to both tools.

This might be a tricky task, as not everything can be configured, in my experience.

Since IDEs can vary and php scripts can be executed by any developer, I would opt for an authoritative code style fixer which can be run from the console and hence tested/verified/applied through git hooks or actions. Additional nice-to-haves could be set in the IDE.

Hence, in the case of conflict, the external php formatter would win. Most IDE's can be configured to run those and they will overwrite the internals settings.

I did not know that it is also possible to format the code via PHPStorm using PHPCS.

In the dev docs we could add a description on how to do that. We could also include respective settings in the code base (e.g. .idea/codeStyles/) - to be removed on release.

Theoretically, we could also just require this setting from the developers and enforce this via GitHub Action.

That would be my recommendation, see above.

Of course, I would prefer if both tools PHPStorm & PHPCS produce the identical output. Then you wouldn't be limited to one tool.

I don't think it's such a big deal, given that the external formatter has precedence.

Do you know any other tools for automatic PHP formatting?

Not extensively. I have my own personal code style for my own projects, that uses a lot of new lines to give space to the eye and make visual navigation easier and source code versioning more atomic. I use the PhpStorm for this.

However, I've also been working with

but don't have enough experience to make an informed recommendation. At one customer the PHP_CodeSniffer would even prevent a commit before it's fixed.

@martin-rueegg
Copy link
Contributor

The reason why I'm asking for your exact standards is, that after checking out

$ gitst
On branch phpcs-admin2
Your branch is up to date with 'upstream/phpcs-admin2'.

$ git1 | head
993cd5b8c Fixed path
cfee14604 Changed PHPCS Action
2ee4833eb Github Action Fix
97bf3c9b3 Test GitHub ActioN
a560f97b3 Fixed syntax problems
bd52c0d60 Fixed user module syntax problems
34f8b2be0 User module code style
1ed20ad99 1: PHPCS User
23e8856e6 Fixed basic view
4725c18a2 Alignments

and then running

$ $HUMHUB_VENDOR_BIN/phpcbf --standard=$HUMHUB_PATH/phpcs.xml $HUMHUB_PATH/protected/humhub/modules/admin

I still ended up with

A TOTAL OF 442 ERRORS WERE FIXED IN 221 FILES

202 files of which are messages, replacing the array notation. The rest is in migrations and tests. Did you exclude those?

Although, most changes are just adding/removing empty lines. And adding brackets after new ClassName-calls.

@martin-rueegg
Copy link
Contributor

#6521 (comment)

I don't know if realistic, but cool would be e.g. a php protected/yii rector/migrate-module /path/to/module which automatically applies some changes from the MIGRATE-DEV.md.

@luke- luke- modified the milestones: v1.16, v1.17 Apr 4, 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.

None yet

3 participants