-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix gutenberg/gutenberg-coding-standards licensing issues #61913
Conversation
Flaky tests detected in 2716212. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9214390920
|
5d89eb5
to
71c3429
Compare
0d70552
to
4ce49e0
Compare
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
06edf90
to
f734bfd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, @anton-vlasenko! I reviewed this PR and left a few comments. There is just one detail that might need to be addressed before merging this PR which is updating a docblock description to reflect the changes made to a method.
I checked the code changes. I don't know about the legal aspects of the licenses, so I don't know if the changes in this PR fix the licensing issues as indicated in the PR title.
test/php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/SinceTagSniff.php
Show resolved
Hide resolved
...tenberg-coding-standards/Gutenberg/Sniffs/CodeAnalysis/ForbiddenFunctionsAndClassesSniff.php
Show resolved
Hide resolved
@@ -65,7 +67,7 @@ public function setCliValues( $filename, $config ) { | |||
if ( ! isset( $GLOBALS['PHP_CODESNIFFER_RULESETS']['Gutenberg'] ) | |||
|| ( ! $GLOBALS['PHP_CODESNIFFER_RULESETS']['Gutenberg'] instanceof Ruleset ) | |||
) { | |||
throw new \RuntimeException( $error_message ); | |||
throw new \RuntimeException( $error_message ); //phpcs:ignore WordPress.Security.EscapeOutput.ExceptionNotEscaped -- this is non-production code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this was not needed before and now it is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The errors likely appeared because I used composer run check-cs
this time, instead of just composer run lint
as I had previously done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the additional details.
Just a nitpick, but I believe the recommended format for the ignore comment includes a space after the two forward slashes (https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file). So, // phpcs:ignore
instead of //phpcs:ignore
. The current version works so feel free to ignore this remark.
Also, not something to be addressed in this PR, but I wonder if composer run check-cs
should be run in CI so that it is easier to catch future coding standard violations like this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. Fixed in 333ccc1.
Also, not something to be addressed in this PR, but I wonder if composer run check-cs should be run in CI so that it is easier to catch future coding standard violations like this one?
Yes, I thought about it, and that would be the right way to do it, in my opinion.
I just don't see it as a high-priority task since there is not a lot of development happening right now (all known issues have been addressed), and it's not likely that the GBCS package will grow significantly in the near future.
cff54f7
to
f1e6818
Compare
Thank you for the code review, @rodrigoprimo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, @anton-vlasenko. I just replied to all of your comments. There is just one nitpick that I found, but feel free to ignore it.
This PR looks good to me. Similar to what I did in previous PRs that I reviewed, I'm not approving this one as I'm not part of the Gutenberg project and could be easily missing something important.
4050b2c
to
f843b61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I see this is good to go. Using the same dual-licensing GPL and MPL as the rest of Gutenberg seems best.
- remove third-party code from the SinceTagSniff::find_docblock() method; - remove third-party code from the SinceTagSniff::is_function_call() method; - remove the SinceTagSniff::find_hook_docblock() method and add the new find_previous_line_token() method (refactoring); - fix typo in the SinceTagSniff::process() method; - remove unused code from bootstrap.php and make variables use snake case;
… environment variable in the context of Gutenberg.
f843b61
to
ded90f0
Compare
Thank you everyone! |
1. change the license to `GPL-2.0-or-later`; 2. remove third-party code from the `SinceTagSniff::find_docblock()` method; 3. remove third-party code from the `SinceTagSniff::is_function_call()` method; 4. remove the `SinceTagSniff::find_hook_docblock()` method and add the new `find_previous_line_token()` method (refactoring); 5. fix typo in the `SinceTagSniff::process()` method; 6. remove unused code from `bootstrap.php` and make variables use snake case; 7. add the `License` section to the `README.md` file. Co-authored-by: anton-vlasenko <antonvlasenko@git.wordpress.org> Co-authored-by: rodrigoprimo <rodrigosprimo@git.wordpress.org> Co-authored-by: azaozz <azaozz@git.wordpress.org>
What?
This PR:
gutenberg/gutenberg-coding-standards
(GBCS) package and removes unused code;GPL-2.0-or-later
license as it provides more freedoms and access to licensees than MIT-license code.Why?
Ensure the package does not use any third-party code.
How?
GPL-2.0-or-later
;SinceTagSniff::find_docblock()
method;SinceTagSniff::is_function_call()
method;SinceTagSniff::find_hook_docblock()
method and add the newfind_previous_line_token()
method (refactoring);SinceTagSniff::process()
method;bootstrap.php
and make variables use snake case;License
section to theREADME.md
file;Testing Instructions
test/php/gutenberg-coding-standards/
.composer update
.composer run check-all
.Testing Instructions for Keyboard
Screenshots or screencast