Skip to content

Conversation

Mamaduka
Copy link
Member

@Mamaduka Mamaduka commented Aug 9, 2021

Changes array_merge with array_replace. The latter will keep the same numeric keys.

Verification example: https://3v4l.org/tlft6

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


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.

@Mamaduka
Copy link
Member Author

Mamaduka commented Aug 9, 2021

/cc @gziolo

@gziolo
Copy link
Member

gziolo commented Aug 9, 2021

I don't know this functionalit. @youknowriad, can you help with the review?

@Mamaduka
Copy link
Member Author

Mamaduka commented Aug 9, 2021

@gziolo apologies. I pinged you since this was editor settings related.

'' => apply_filters( 'default_page_template_title', __( 'Default template' ), 'rest-api' ),
),
$available_templates
)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the difference could be that array_merge includes the default template even if not already available in $available_templates. Do we expect it to be always available?

Copy link
Member Author

Choose a reason for hiding this comment

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

The array_replace function also does that. Updated REPL example - https://3v4l.org/sgsQg.

I think I need to swap argument positions so the "Default template" is always the first item in the array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok weird, that replace does that as well.

Now I'm curious what the expected result if "" key is present from the available_templates array? Do we expect to just add it if missing or do we expect to replace its label as well? (which would make the position change)

Personally, I'm not familiar with this code to guess the original intent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think wp_get_theme()->get_page_templates( get_post( $post->ID ) ); will ever return template with "" as a key.

My best guess about the "Default template" option is that it represents the state when a template isn't set for page/post, and it defaults to whatever WP resolves in the template hierarchy.

Copy link
Contributor

@youknowriad youknowriad 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

@Mamaduka
Copy link
Member Author

Mamaduka commented Aug 9, 2021

Thanks for the review, @youknowriad.

I think I need to swap argument positions so the "Default template" is always the first item in the array.

I will push this change in few minutes.

@SergeyBiryukov
Copy link
Member

Thanks for the PR! Merged in https://core.trac.wordpress.org/changeset/51595.

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

Successfully merging this pull request may close these issues.

4 participants