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: describe return type of _get_block_template_file(). #4388

Closed
wants to merge 6 commits into from
Closed

Docs: describe return type of _get_block_template_file(). #4388

wants to merge 6 commits into from

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Apr 27, 2023

It was brought up at https://core.trac.wordpress.org/ticket/57756#comment:16 that some inline comments introduced at #4097 could be graduated to a PHP DocBlock for better maintenance: use single place for documenting them as well as enable the existing automated docs to pick them up.

This PR removes the inline comments and makes them part of the _get_block_template_file().


SVN commit message:

Docs: describe return type of `_get_block_template_file()`.

Props: desrosj, mukesh27, costdev, johnbillion.
Fixes #57756.

@oandregal
Copy link
Member Author

An alternative I considered was to make them part of the functions _add_block_template_part_area_info() and _add_block_template_info for wp_template_part and wp_template, respectively.

The role of these functions is to augment the metadata of the template (title + area, or title + postTypes) provided as a parameter. These functions only work with a subset of the template properties (slug as input). It doesn't feel right to have them document what the respective template structure is for a lack of a better place.

@oandregal oandregal requested a review from desrosj April 27, 2023 11:12
@oandregal oandregal self-assigned this Apr 27, 2023
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
@oandregal
Copy link
Member Author

@mukeshpanchal27 @desrosj is there any other feedback you want addressed?

Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @oandregal! Just a couple of minor suggestions. The rest looks good to me!

src/wp-includes/block-template-utils.php Outdated Show resolved Hide resolved
src/wp-includes/block-template-utils.php Outdated Show resolved Hide resolved
oandregal and others added 2 commits May 8, 2023 10:07
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
@oandregal
Copy link
Member Author

@mukeshpanchal27 @costdev @desrosj anyone available to give me a green check on this one, so I can commit?

Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @oandregal! LGTM 👍

Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

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

Looks good to me, just left a small nit. Sorry for the delay!

* @type string $path Template file path.
* @type string $theme Theme slug.
* @type string $type Template type.
* @type string $title Optional. Template title.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find myself wanting all of the optional items at the end of the list. This area of the standards looks relevant here, but there's no mention that optional parameters should all be at the end.

If order is not important, let's just move $area ahead of $title to group optional parameters together.

Copy link
Contributor

@costdev costdev May 8, 2023

Choose a reason for hiding this comment

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

I don't see any uses in Core that depend on the order. This is a private function, but even still, I checked if we know any plugins/themes that use it. The only two mentions of it are in Gutenberg, whose implementation doesn't seem to depend on order, and a small plugin that defines the function itself, so it seems reordering is somewhat "safe".

I'm not sure whether it's better to put optional elements at the end, or to accurately document the return value should we ever (for some reason) use array_values() on it. An alternative could be to reorder the actual return value as well as the documentation, though that seems more involved that we might want to do here.

oandregal and others added 2 commits May 9, 2023 13:16
@oandregal
Copy link
Member Author

@oandregal oandregal closed this May 11, 2023
@oandregal oandregal deleted the fix/docs-for-block-template-types branch May 11, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants