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

Docs: Don't recommend using short array syntax in WP_HTML_Tag_Processor #47958

Merged
merged 1 commit into from
Feb 10, 2023

Conversation

aristath
Copy link
Member

What?

Docs for methods in the WP_HTML_Tag_Processor object use a short array syntax. However, that syntax is not allowed in WP. This PR changes the examples to use the long array syntax.

Testing Instructions

Nothing to test. This is a simple docs change.

@aristath aristath added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 10, 2023
@aristath aristath enabled auto-merge (squash) February 10, 2023 09:21
@aristath aristath merged commit 0f604e5 into trunk Feb 10, 2023
@aristath aristath deleted the fix/html-parser-docs branch February 10, 2023 09:29
@github-actions github-actions bot added this to the Gutenberg 15.2 milestone Feb 10, 2023
@github-actions
Copy link

Flaky tests detected in 4ea9e3c.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4142386379
📝 Reported issues:

pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Feb 10, 2023
…tion.

Per [https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#declaring-arrays WordPress PHP Coding Standards]:
> Using long array syntax ( `array( 1, 2, 3 )` ) for declaring arrays is generally more readable than short array syntax ( `[ 1, 2, 3 ]` ), particularly for those with vision difficulties. Additionally, it’s much more descriptive for beginners.
> 
> Arrays must be declared using long array syntax.

Original PR from Gutenberg repository:
* [WordPress/gutenberg#47958 #47958 Docs: Don't recommend using short array syntax in WP_HTML_Tag_Processor]

Follow-up to [55203], [55206].

Props aristath, poena.
Fixes #57691.

git-svn-id: https://develop.svn.wordpress.org/trunk@55304 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Feb 10, 2023
…tion.

Per [https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#declaring-arrays WordPress PHP Coding Standards]:
> Using long array syntax ( `array( 1, 2, 3 )` ) for declaring arrays is generally more readable than short array syntax ( `[ 1, 2, 3 ]` ), particularly for those with vision difficulties. Additionally, it’s much more descriptive for beginners.
> 
> Arrays must be declared using long array syntax.

Original PR from Gutenberg repository:
* [WordPress/gutenberg#47958 #47958 Docs: Don't recommend using short array syntax in WP_HTML_Tag_Processor]

Follow-up to [55203], [55206].

Props aristath, poena.
Fixes #57691.
Built from https://develop.svn.wordpress.org/trunk@55304


git-svn-id: http://core.svn.wordpress.org/trunk@54837 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Feb 10, 2023
…tion.

Per [https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#declaring-arrays WordPress PHP Coding Standards]:
> Using long array syntax ( `array( 1, 2, 3 )` ) for declaring arrays is generally more readable than short array syntax ( `[ 1, 2, 3 ]` ), particularly for those with vision difficulties. Additionally, it’s much more descriptive for beginners.
> 
> Arrays must be declared using long array syntax.

Original PR from Gutenberg repository:
* [WordPress/gutenberg#47958 #47958 Docs: Don't recommend using short array syntax in WP_HTML_Tag_Processor]

Follow-up to [55203], [55206].

Props aristath, poena.
Fixes #57691.
Built from https://develop.svn.wordpress.org/trunk@55304


git-svn-id: https://core.svn.wordpress.org/trunk@54837 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@ntsekouras ntsekouras removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 13, 2023
@DaisyOlsen DaisyOlsen added the [Type] Developer Documentation Documentation for developers label Feb 14, 2023
* | Find next image tag containing the `fullwidth` CSS class. | `$tags->next_tag( [ 'tag_name' => 'img', 'class_name' => 'fullwidth' ] );` |
* | Find next image tag. | `$tags->next_tag( array( 'tag_name' => 'img' ) );` |
* | Find next tag containing the `fullwidth` CSS class. | `$tags->next_tag( array( 'class_name' => 'fullwidth' ) );` |
* | Find next image tag containing the `fullwidth` CSS class. | `$tags->next_tag( array( 'tag_name' => 'img', 'class_name' => 'fullwidth' ) );` |
Copy link
Member

Choose a reason for hiding this comment

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

@aristath looks like the formatting of the table was messed up here during this change. would you mind updating to fix it?

as a side-note, I know we had held a discussion about the use of the shorthand syntax in comments vs. in code. I guess there are still some differences in opinion around about that. would have been nice to be at least pinged in this patch so we could have caught wind of the change.

note that there are a couple existing uses of array shorthand syntax in comments in rest-api.php, as well as prevalent use in wp-includes/Requests. I suspect you will want to remove those too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think documentation should include code examples that cannot be used. Wether it is in a comment or not.

Copy link
Member

Choose a reason for hiding this comment

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

@carolinan my comment is not arguing whether this was a positive or negative change, but pointing out a process note:

  • it would be courteous and cooperative to include people who have recently been doing a lot of work within a file to poke them in a PR before rapidly proposing and merging changes.
  • the style changes in this PR are not comprehensive, leaving in place existing examples of the style being changed, which raises the question "how do we decide if we change the style of existing accepted code?"

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you are right.

Copy link
Member Author

@aristath aristath Mar 7, 2023

Choose a reason for hiding this comment

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

looks like the formatting of the table was messed up here during this change. would you mind updating to fix it?

Thank you for catching that! I submitted a followup on #48816 👍

it would be courteous and cooperative to include people who have recently been doing a lot of work within a file to poke them in a PR before rapidly proposing and merging changes.

You are 100% correct. Please accept my apologies, it didn't cross my mind.

I know we had held a discussion about the use of the shorthand syntax in comments vs. in code. I guess there are still some differences in opinion around about that.

Yeah... Personally, I prefer the short syntax, and I wish we could use it everywhere. That's what I always use when writing code. However, the official WP Coding Standards recommend using the long-array syntax. As a result, when someone copy-pastes code from these examples/docs and tries to use them in a project that uses the WordPress Coding Standards (like for example Gutenberg), the IDE throws CS notices/errors about the use of the short array syntax, and automated tests won't pass. That's why this change was made...

Copy link
Member

Choose a reason for hiding this comment

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

thanks all

VenusPR added a commit to VenusPR/Wordpress_Richard that referenced this pull request Mar 9, 2023
…tion.

Per [https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#declaring-arrays WordPress PHP Coding Standards]:
> Using long array syntax ( `array( 1, 2, 3 )` ) for declaring arrays is generally more readable than short array syntax ( `[ 1, 2, 3 ]` ), particularly for those with vision difficulties. Additionally, it’s much more descriptive for beginners.
> 
> Arrays must be declared using long array syntax.

Original PR from Gutenberg repository:
* [WordPress/gutenberg#47958 #47958 Docs: Don't recommend using short array syntax in WP_HTML_Tag_Processor]

Follow-up to [55203], [55206].

Props aristath, poena.
Fixes #57691.
Built from https://develop.svn.wordpress.org/trunk@55304


git-svn-id: http://core.svn.wordpress.org/trunk@54837 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants