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

Block Library: Try to standardize PHP function names used #20085

Merged
merged 3 commits into from Feb 10, 2020

Conversation

@gziolo
Copy link
Member

gziolo commented Feb 7, 2020

Description

This PR tries to standardize all functions names used in PHP files created for core blocks. I don't know how much it is still possible to do, but we should double-check and update at least all those blocks that are new to WordPress 5.4.

Updated blocks

Pre 5.4

Needs to be confirmed:

  • Categories
  • Latest Comments

5.4 and beyond

It's safe to update:

  • Legacy Widget
  • Navigation
  • Social Icon

Testing

I ensured that all listed blocks can be inserted in the post content and they render on the frontend.

@gziolo gziolo self-assigned this Feb 7, 2020
@gziolo gziolo force-pushed the try/block-library-php-function-names branch 2 times, most recently from 8a778a7 to 3ffd816 Feb 7, 2020
@gziolo gziolo requested review from mcsf and youknowriad Feb 7, 2020
@gziolo gziolo force-pushed the try/block-library-php-function-names branch from 3ffd816 to d37df94 Feb 7, 2020
@gziolo gziolo marked this pull request as ready for review Feb 7, 2020
@gziolo gziolo requested review from ajitbohra, mkaz, Soean and talldan as code owners Feb 7, 2020
@gziolo gziolo requested review from azaozz and aduth Feb 7, 2020
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Feb 7, 2020

@aduth – if you would be able to land all those changes, we might also consider simplifying the logic in webpack that modifies names of the functions to a simple string replace block_core_ -> block_gutenberg_.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 7, 2020

Related: #20039 , #18589 (comment)

For functions already present as of WordPress 5.3.x, is it even possible to change the names without breaking some backward-compatibility commitment?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 7, 2020

What is the standard we're trying to encourage here? I think the inconsistencies we've seen thus far can be explained at least in part by not having a clear idea / documentation for what we want.

My comment at #18589 (comment), while somewhat rambling, was an exploration of this. I see some of the same patterns here.

To clarify, is the idea something like always using block_core_[block_name] as a prefix? I note an exception is made for the render functions, but only in that it prefixes render_ to the above pattern. Should that be exclusive to render function? Will other verb prefixes be sensible? Or should we try to just keep it simple and force this convention? (a "render" exception could still be reasonable)

Case in point:

function block_core_social_link_get_name( $site ) {

Technically, if you follow the convention of the render prefix, this could just as well be:

get_block_core_social_link_name

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Feb 7, 2020

Why aren't we using PHP namespaces?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 7, 2020

@ZebulanStanphill My understanding is it's an unsettled question in core standards, since the feature only became available as of the recent-ish minimum PHP version bump to 5.6. It's not something I think we can hope to address as part of the immediate goals here, though it would be worth being mindful of some future interoperability.

https://make.wordpress.org/core/2019/03/26/coding-standards-updates-for-php-5-6/

Namespaces

Namespaces are a neat way to encapsulate functionality, and are a common feature in modern PHP development practices. As we’ve discovered in the past, however, introducing namespaces to the WordPress codebase is a difficult problem, which will require careful architecture and implementation.

Side note: there’s currently no timeline for introducing namespaces to WordPress Core, expressions of interest are welcome. 🙂

Coding Standards Proposal

At this time, namespaces must not be used in WordPress Core.

@mcsf mcsf mentioned this pull request Feb 7, 2020
0 of 6 tasks complete
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Feb 7, 2020

For functions already present as of WordPress 5.3.x, is it even possible to change the names without breaking some backward-compatibility commitment?

I have no idea. That's why I divided the list of updated blocks to post and pre 5.3.0 release.

What is the standard we're trying to encourage here? I think the inconsistencies we've seen thus far can be explained at least in part by not having a clear idea / documentation for what we want.

I followed the patterns that exist already in the codebase. I think they were born naturally. I'm not aware of any written recommendation.

My comment at #18589 (comment), while somewhat rambling, was an exploration of this. I see some of the same patterns here.

To clarify, is the idea something like always using block_core_[block_name] as a prefix? I note an exception is made for the render functions, but only in that it prefixes render_ to the above pattern. Should that be exclusive to render function? Will other verb prefixes be sensible? Or should we try to just keep it simple and force this convention? (a "render" exception could still be reasonable)

Yes, render_ prefix is the only exception. It's something that is used with filters so I doubt we can do anything about it. However, I also like the proposal you share with using [verb]_[block_name] :)

Technically, if you follow the convention of the render prefix, this could just as well be:

get_block_core_social_link_name

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 7, 2020

Trying to think about what might be the most flagrant violations for blocks we have known control over (the ones you consider as 5.4 and beyond, blocks pending for 5.4).

These ones should incorporate block_core_ like elsewhere:

  • core_social_link_get_icon
  • core_social_link_get_name
  • core_social_link_sites
  • core_social_link_get_icon
  • core_block_navigation_build_css_colors
  • core_block_navigation_build_css_font_sizes
  • core_block_navigation_empty_navigation_links_recursive
  • core_block_navigation_render_submenu_icon
  • render_widget_by_id

These to follow the convention of render_block_core_[block_name]:

  • render_core_social_link -> render_block_core_social_link
  • render_block_navigation -> render_block_core_navigation
  • render_block_legacy_widget -> render_block_core_legacy_widget

To clarify, you have made most of these corrections already. Just highlighting the specifics about what I think can be considered as incorrect about them.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Feb 7, 2020

For functions already present as of WordPress 5.3.x, is it even possible to change the names without breaking some backward-compatibility commitment?

I have no idea. That's why I divided the list of updated blocks to post and pre 5.3.0 release.

As far as I know, we can not change the names of functions part of WordPress core.

@azaozz

This comment has been minimized.

Copy link
Contributor

azaozz commented Feb 7, 2020

As far as I know, we can not change the names of functions part of WordPress core.

It is possible, but the old names should be deprecated. True, it's not "ideal" to deprecate functions just for that reason, but on the other hand having consistent, easy to understand and follow naming scheme would be really nice.

Always using block_core_[block_name] as a prefix seems to make a lot of sense. Thinking that a "one time" renaming of functions that exist in WP 5.3 would be warranted to implement good naming scheme.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 7, 2020

we might also consider simplifying the logic in webpack that modifies names of the functions to a simple string replace block_core_ -> block_gutenberg_.

My recommendation on this specific point would be:

  • If we keep wp_latest_comments_draft_or_post_title and choose not to rename it, then no changes should be made to the Webpack transform.
  • If wp_latest_comments_draft_or_post_title is renamed, we should remove .replace( /^wp_/, '' ) from this line of code (and update the leading comment accordingly).

I don't think we ought to substitute "core", and I think we should continue to prefix these with gutenberg_ in the plugin, as is conventional for plugin function naming. It's also not strictly inaccurate to say that the Gutenberg plugin is operating with the "core blocks".

@aduth aduth force-pushed the try/block-library-php-function-names branch from d37df94 to b57011e Feb 10, 2020
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 10, 2020

While we might have the option to rename existing functions as part of a deprecation, that option will remain open to us in the next release as well (WordPress 5.5.0), so I don't think it's the most pressing concern here.

Instead, I've rebased the branch to update only those blocks pending to be new additions as part of WordPress 5.4.0, to try to conform to a more standardized naming convention.

My expectation is that this is:

  • block_core_[snake_case_name]_[verb]_[descriptor]
  • With two exceptions: render_block_core_[snake_case_name], register_block_core_[snake_case_name]

Per the original comment, this is only expected to impact:

  • Legacy Widget
  • Navigation
  • Social Icon
Copy link
Member

jorgefilipecosta left a comment

The PR tested well for, I tried the three changed blocks without any error on the editor and front end 👍

@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Feb 10, 2020

Thanks, @aduth for helping with this PR. It makes sense to proceed as you explained 💯

@aduth aduth merged commit 9ac8c66 into master Feb 10, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@aduth aduth deleted the try/block-library-php-function-names branch Feb 10, 2020
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 10, 2020
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 10, 2020

I retained commit history for the original approach, in case it is needed for future reference: 7b608da

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.