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

Update class-wp-style-engine-processor.php #4902

Closed
wants to merge 13 commits into from

Conversation

Rajinsharwar
Copy link

Setting the default optimization option to false.

Trac ticket: https://core.trac.wordpress.org/ticket/58811


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@audrasjb
Copy link
Contributor

PHP Unit tests are failing, I think they need to be modified accordingly.

@ramonjd
Copy link
Member

ramonjd commented Jul 28, 2023

Thanks for the patch.

There's a corresponding Gutenberg issue:

I think it should be addressed there unless there's urgency in Core.

@ramonjd
Copy link
Member

ramonjd commented Jul 28, 2023

Thanks for this patch!

Here's an equivalent Gutenberg PR with tests:

WordPress/gutenberg#53085

Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

Thank you for this PR.

I just left a couple of notes related to annotations.

@SergeyBiryukov
Copy link
Member

SergeyBiryukov commented Aug 1, 2023

Thanks for the PR! If optimize is no longer enabled by default, then a few other DocBlocks will have to be updated as well, see r55820 for all the instances that should be changed to Default false.

@Rajinsharwar
Copy link
Author

Thanks for letting me know @SergeyBiryukov. Just updated the other DocBlocks.

@@ -86,19 +86,21 @@ public function add_rules( $css_rules ) {
*
* @since 6.1.0
*
* Since 6.3.0 Optimization is no longer the default.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a correct since mention (should be something like @since 6.3.0 $optimize now defaults to false.) but I'm not sure we need a since mention at the first place.
Any thought @SergeyBiryukov?

Copy link
Author

Choose a reason for hiding this comment

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

I think we should go with something like: @audrasjb

 * Optimization is no longer the default
 * @ticket 58811

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the proposal @Rajinsharwar, but this is not a correct way to use since mentions.
Please check Docblock standards: https://developer.wordpress.org/coding-standards/inline-documentation-standards/php/#since-section-changelogs

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for that @audrasjb, just updated it. Can you take a look?

Rajinsharwar and others added 2 commits August 3, 2023 17:09
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
@tellthemachines
Copy link
Contributor

Committed in r56574.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants