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

Switch require_once to require for loading asset files #18599

Merged
merged 2 commits into from Nov 19, 2019
Merged

Conversation

@mkaz
Copy link
Member

mkaz commented Nov 19, 2019

Description

If the function gets called twice the dependency array will reset due to the require_once() return value. The asset_file is being included to a variable, not loaded like a class.

Fixes #18596

I switched to use include() because the function is more clear what is happening. Plus will not throw a fatal if the file does not exist, it simply returns false with a warning. This also allows removing the extra check for file existence.

Additionally, if the index.asset.php file does not exist, then we have a build issue since it is part of the bundle. We shouldn't need to detect if it exists and swallow that error silently. This PR will properly throw a warning as it should with a missing file that is needed.

How has this been tested?

Confirmed loading still works as expected.

Function reference: PHP include documentation

Types of changes

Fixes loading of asset_file and dependencies.

@mkaz mkaz requested a review from TimothyBJacobs as a code owner Nov 19, 2019
@mkaz mkaz requested a review from mdawaffe Nov 19, 2019
Copy link
Member

pento left a comment

Given that WPCS' WordPress-Extra ruleset warns for using include instead of require, and that adding WordPress-Extra to Gutenberg is on the cards (#18502), we should stick with require here. 🙂

@mkaz

This comment has been minimized.

Copy link
Member Author

mkaz commented Nov 19, 2019

@pento Probably not worth going into here, but do you know the background for what's wrong with include()?

Nevermind, I followed the links.

@mkaz

This comment has been minimized.

Copy link
Member Author

mkaz commented Nov 19, 2019

The code required for this scenario is just recreating the include() function.

The ternary

$asset = file_exists( $asset_file )
    ? require( $asset_file )
    : null;

just recreates what the include() function does, see documentation. Though include returns false instead of null, but for the rest of the function it doesn't make a difference.

Also this seems to be a much clearer implementation:

$asset = include( $asset_file );

So I politely disagree with the coding standard. ;-)

@pento

This comment has been minimized.

Copy link
Member

pento commented Nov 19, 2019

Those two implementations are different. include() emits a PHP warning if the file doesn't exist, the file_exists() check will prevent the first implementation from generating a PHP error if the file doesn't exist.

@mkaz

This comment has been minimized.

Copy link
Member Author

mkaz commented Nov 19, 2019

Agreed, it does also throw a warning, but as I stated in the top description this is a feature, because it should warn that files that are supposed to be part of the built Gutenberg don't exist.

The previous implementation the build could be broken with no errors or warnings.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 19, 2019

There is also this documentation section:
https://github.com/WordPress/gutenberg/tree/master/packages/dependency-extraction-webpack-plugin#wordpress

$script_asset      = file_exists( $script_asset_path )
	? require( $script_asset_path ) 
	: array( 'dependencies' => array(), 'version' => filemtime( $script_path ) );

Should this example be also updated?

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Nov 19, 2019

The move from include_once to require_once was deliberate: bd0a56c. As @gziolo and I discussed at the time, there was a point in making all of this quite explicit and ready to blow up. Similarly, we preferred file_exists over is_readable: 0595f79#r324492829.

It could be argued that the need to run gutenberg_register_packages_scripts() twice is a code smell, no?

@mkaz

This comment has been minimized.

Copy link
Member Author

mkaz commented Nov 19, 2019

@mcsf It won't blow up though because it checks for the file_exists first, it just silently fails.

We should come to a consensus on what we think it should be and update the spots, the Gutenberg examples repo uses the include function:
https://github.com/WordPress/gutenberg-examples/blob/master/01-basic-esnext/index.php#L33

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Nov 19, 2019

It won't blow up though because it checks for the file_exists first, it just silently fails.

Sure (and I don't remember the details leading to checking for file existence first), but it will fail more aggressively in other circumstances: incorrect read permissions on the file, and — the present issue — a duplicated script registration.

Nevertheless, I think this question still stands:

It could be argued that the need to run gutenberg_register_packages_scripts() twice is a code smell, no?

@mkaz

This comment has been minimized.

Copy link
Member Author

mkaz commented Nov 19, 2019

It could be argued that the need to run gutenberg_register_packages_scripts() twice is a code smell, no?

Yes, the function should not be called twice, but it is for whatever reason.

The function can also be coded in a way that if it is called twice it doesn't break.

I switched it over since it seems like the consensus is to use require

@mdawaffe

This comment has been minimized.

Copy link

mdawaffe commented Nov 19, 2019

It could be argued that the need to run gutenberg_register_packages_scripts() twice is a code smell, no?

Yes - I think it speaks to a deeper Core weirdness and/or some sort of doing_it_wrong.

But I agree with @mkaz: using require means this function can be called multiple times, regardless of the "need". require is also faster than require_once, so, if called only once as expected, this is still an improvement. (Less objectively, require seems more appropriate here since it's not loading library code but data that is meant to be executed on the fly.)

So did this issue come up because of some strangeness somewhere else in the stack? Yes.

But is this change an improvement regardless of how it came up? I claim yes :)

@pento
pento approved these changes Nov 19, 2019
@pento pento added this to the Gutenberg 7.0 milestone Nov 19, 2019
@pento pento added the [Type] Bug label Nov 19, 2019
@pento pento changed the title Switch require_once to include for asset file Switch require_once to require for loading asset files Nov 19, 2019
@pento pento merged commit 33ea2e4 into master Nov 19, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@pento pento deleted the fix/18596-asset-file branch Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.