Use get_theme_file_path function.

[spacedmonkey]

Use get_theme_file_path function to stop replicated code.

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

---

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.

[felixarntz]

@spacedmonkey I think we should either leave this as is and continue to rely on the is_readable logic, or we should consistently switch this to use file_exists. Right now the PR here results in a mix of both which does not make sense to me.

I don't have a strong opinion either way. If there's no particular reason why the theme.json related code started using is_readable instead of file_exists, I'd be onboard with changing this code to use get_theme_file_path(), but then also consistently use file_exists.

[felixarntz]

Apparently there's some whitespace here that causes a PHPCS error?

[spacedmonkey]

@mukeshpanchal27 @felixarntz

I have include another commit, to also use the theme_file_path filter in theme json resolver. If we are going to implement using this filter in one place, it needs to be done in all.

think of this use case.

• Developer, has a filter that filters the location of the theme.json file.
• Calls wp_theme_has_theme_json, get return true.
• But then, get_theme_data in theme json resolver, use get_file_path_from_theme that does not respect that.

[hellofromtonya]

Why is_readable() vs file_exists()?

is_readable — Tells whether a file exists and is readable
Returns true if the file or directory specified by filename exists and is readable, false otherwise.
https://www.php.net/manual/en/function.is-readable.php

file_exists — Checks whether a file or directory exists
Returns true if the file or directory specified by filename exists; false otherwise.
https://www.php.net/manual/en/function.file-exists.php

is_readable() then is an extra safety measure to ensure the file is usable for reading it into memory.

[spacedmonkey]

is_readable() then is an extra safety measure to ensure the file is usable for reading it into memory.

I recommend using is_readable, as this what is correctly in core.

I think using the theme_file_path is extremely useful for developers.

I would like to unblock this, can I get a review of this.

@hellofromtonya @oandregal @felixarntz

[oandregal]

My main comment is that these changes should happen in Gutenberg first. This is something I have recurrently mentioned, and it takes time and energy for everyone. I feel I sound like a broken record. I'm sorry about that. Is there something I can do to clarify this, so nobody needs to waste energy/time on this again?

[oandregal]

For full transparency: other than I already commented back in February, I don't fully understand how this is impactful (performance, users, etc.). I feel others would be better equipped than I am to help figure this out.

[spacedmonkey]

I feel I sound like a broken record. I'm sorry about that.

Can you point me in the direction of the documented process for this? @oandregal

[oandregal]

Can you point me in the direction of the documented process for this?

As far as I know, it has been good etiquette among people that work together, and it has been transmitted PR to PR in comments. I have been personally doing this for months, if not a year. I know @azaozz and @hellofromtonya have been sharing this as well in other channels, they will have more context as to where.

I don't know that there is a handbook page anywhere that states this. I'm happy to help writing one down if that's useful to help us focus on solving user's problems.

[johnbillion]

Following on from the above, is this process documented in a handbook yet? If there are files in core that need to be modified first in Gutenberg then ideally there should be a test that fails when they're modified in a core PR.

[audrasjb]

The logic looks good to me 👍

[costdev]

Thanks for the PR @spacedmonkey! As your plan is to backport this to Gutenberg in this PR, everything looks good to me here - and nice one using is_readable() over file_exists() 👍

[felixarntz]

Thanks @spacedmonkey, LGTM!