Navigation: Namespace PHP functions by obenland · Pull Request #18589 · WordPress/gutenberg

obenland · 2019-11-18T19:03:30Z

Description

Adds gutenberg_ prefix to navigation block functions.
These functions will get merged to Core verbatim, so let's make sure functions like build_css_colors() don't clash.

For some reason Gutenberg doesn't recognize registration callback function names that don't start with register_block_?!

How has this been tested?

Loaded a navigation block in editor and frontend on a local install and made sure it doesn't cause errors.

Checklist:

My code is tested.
My code follows the WordPress code style.

obenland · 2019-11-18T19:04:10Z

packages/block-library/src/navigation-menu/index.php

 */
-function register_block_core_navigation_menu() {
-
+function register_block_core_navigation() {


Not sure why this can't be prefixed with gutenberg_?

They are prefixed, but not in the source. The prefix is added at build time:

https://github.com/WordPress/gutenberg/blob/6fc0b70/webpack.config.js#L129-L139

You can see this in the build output:

https://plugins.trac.wordpress.org/browser/gutenberg/trunk/build/block-library/blocks/navigation-menu.php

It's done this way because core copies these files directly from Gutenberg:

https://github.com/WordPress/wordpress-develop/blob/4ebaf2b/tools/webpack/packages.js#L96-L108

Since Gutenberg needs to replace / augment the implementation of core blocks, and in order to avoid redeclaring the function, a prefix is added automatically.

As far as what I was suggesting with my comments in the previous pull request, we should write the source as though we expect these functions to exist one day in core, and as such consider whether build_css_colors is named specific enough given that it's used only by this one block (and not a generalized utility for building CSS colors).

I did not know that Gutenberg would do that. In that case the prefix should probably change to something like block_{blockname}_*

draganescu

This works OK after the refactor of the function names. Will need to be rebased if #18607 is merged in first.

getdave · 2020-01-08T09:18:19Z

Is this just waiting on something to merge?

draganescu · 2020-01-08T11:48:26Z

Is this just waiting on something to merge?
A rebase and a failing test. Will try to do the rebase myself.

draganescu

@getdave @obenland I basically copied these changes to the new location of the navigation index.php file as this PR got too far behind for a normal rebase. Please take it for a spin (I tested and things still work as expected) so it doesn't end up as me reviewing my code :D

aduth

I should have been clearer in my comment at #18589 (comment), but: We do not want any Gutenberg-specific prefixes here, because this code will be used verbatim as the core implementation. But, as such, for functions which are being added to the global PHP scope, they should be named in such a way that they are unique and sensible in the context of WordPress, because they will effectively become part of the WordPress public API.

Some prior art:

https://github.com/WordPress/WordPress/blob/cef48881f00c61e0630fff16c7b28ecb43ca309d/wp-includes/blocks/categories.php#L68

So for something like build_css_colors, naming it in a way which makes it clearer that it's intended to be used specifically as a utility function for the navigation block (if it can't otherwise be made into a generic utility). Something like get_navigation_block_css_colors.

It could also be something worth considering to mark as an @access private internal function. I'm not really sure the guidelines around this, but considering that these functions don't really have value outside their use by the block itself, it would seem to make sense to me.

https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#phpdoc-tags

apeatling · 2020-01-28T17:12:08Z

@obenland Plans for this PR?

obenland · 2020-01-30T21:31:34Z

That's a good question. In an ideal world I'd introduce a namespace Editor; declaration and be done with it.

they should be named in such a way that they are unique and sensible in the context of WordPress, because they will effectively become part of the WordPress public API.

That tells me we should prefix them with wp_, although I believe that would counter-act the paradigm of keeping Gutenberg CMS agnostic?

Is the solution to introduce a new editor_ namespace?

aduth · 2020-01-31T16:29:02Z

That tells me we should prefix them with wp_, although I believe that would counter-act the paradigm of keeping Gutenberg CMS agnostic?

Is the solution to introduce a new editor_ namespace?

For context, I'm not really familiar if one of the original goals with this block was that it not depend on anything with how WordPress deals with menus, etc. But at the point a block includes a PHP file, it seems to me there's a pretty high expectation about where that block is going to be able to run (namely, WordPress). And if it doesn't require any WordPress functions, is it even dynamic? And if it's not dynamic, then why does the PHP file exist at all (vs. saving static markup from the block's save JavaScript)? In any case, inside a PHP file I'm not sure we really need to act as if WordPress doesn't exist, or that its functions are unavailable to us. Even if used outside WordPress, it would still be correct to refer to these block implementations as being part of the "WordPress block library", so at least on the point of naming, I don't think it's an inaccurate prefix to use.

To me, the agnosticism is more about removing biases about how we tend to think of content, avoiding unnecessary dependencies, and maximizing reusability. I also think that as long as we treat block types as individual units, these decisions and compromises can be made on a block-by-block basis. Maybe it's fine this block depends on WordPress, and another implementation of a Navigation block could work entirely differently.

Back to the original question:

There is some prior art of wp_
- wp_latest_comments_draft_or_post_title
- It's even anticipated by (and removed in) Gutenberg's own build

So it would be okay enough I think. There might be better alternatives. Even in the context of WordPress core code, I tend to see the wp_ used inconsistently, perhaps when better names exist.

Consider as well the name and structure of the Navigation block registration function:

gutenberg/packages/block-library/src/navigation/index.php

Line 165 in 32c5bc4

function register_block_core_navigation() {

There could be something of a pattern here around [verb]_block_[block_namespace]_[block_slug]_[extra]

So something like:

build_block_core_navigation_markup
build_block_core_navigation_css_font_sizes
build_block_core_navigation_color_attributes

Maybe not the most expressive, but they at least align to a consistent pattern to follow.

The prior art is a bit all over the place, to be honest.

gutenberg/packages/block-library/src/social-link/index.php

Line 106 in 539b1fc

function core_social_link_get_icon( $site ) {

gutenberg/packages/block-library/src/categories/index.php

Line 68 in 539b1fc

function build_dropdown_script_block_core_categories( $dropdown_id ) {

aduth · 2020-02-07T13:56:51Z

Appears that this was handled separately in #20039.

obenland added [Type] Task Issues or PRs that have been broken down into an individual action to take [Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Block] Navigation Affects the Navigation Block labels Nov 18, 2019

obenland requested review from Soean, ajitbohra, jorgefilipecosta and talldan as code owners November 18, 2019 19:03

obenland self-assigned this Nov 18, 2019

obenland requested a review from aduth November 18, 2019 19:03

obenland commented Nov 18, 2019

View reviewed changes

draganescu approved these changes Nov 19, 2019

View reviewed changes

Namespaces the navigation serverside rendering functions

32c5bc4

draganescu force-pushed the update/namespace branch from d98437c to 32c5bc4 Compare January 8, 2020 12:11

draganescu self-requested a review January 8, 2020 12:13

draganescu requested changes Jan 8, 2020

View reviewed changes

aduth requested changes Jan 8, 2020

View reviewed changes

obenland mentioned this pull request Jan 15, 2020

Do not output navigation links with empty labels #19652

Merged

4 tasks

aduth mentioned this pull request Feb 7, 2020

Fix: Navigation block: Make function names specific #20039

Merged

aduth closed this Feb 7, 2020

aduth deleted the update/namespace branch February 7, 2020 13:56

aduth mentioned this pull request Feb 7, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navigation: Namespace PHP functions#18589

Navigation: Namespace PHP functions#18589
obenland wants to merge 1 commit intomasterfrom
update/namespace

obenland commented Nov 18, 2019

Uh oh!

obenland Nov 18, 2019 •

edited

Loading

Uh oh!

aduth Nov 18, 2019

Uh oh!

obenland Nov 18, 2019

Uh oh!

draganescu left a comment

Uh oh!

getdave commented Jan 8, 2020

Uh oh!

draganescu commented Jan 8, 2020 •

edited

Loading

Uh oh!

draganescu left a comment

Uh oh!

aduth left a comment

Uh oh!

apeatling commented Jan 28, 2020

Uh oh!

obenland commented Jan 30, 2020

Uh oh!

aduth commented Jan 31, 2020

Uh oh!

aduth commented Feb 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

obenland commented Nov 18, 2019

Description

How has this been tested?

Checklist:

Uh oh!

obenland Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

obenland Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

draganescu left a comment

Choose a reason for hiding this comment

Uh oh!

getdave commented Jan 8, 2020

Uh oh!

draganescu commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

draganescu left a comment

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

apeatling commented Jan 28, 2020

Uh oh!

obenland commented Jan 30, 2020

Uh oh!

aduth commented Jan 31, 2020

Uh oh!

aduth commented Feb 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

obenland Nov 18, 2019 •

edited

Loading

draganescu commented Jan 8, 2020 •

edited

Loading