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

Parameter 1 to gutenberg_register_packages_styles() expected to be a reference, value given #19403

Closed
tomjn opened this issue Jan 4, 2020 · 3 comments
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Status] Needs More Info Follow-up required in order to be actionable. [Type] Code Quality Issues or PRs that relate to code quality

Comments

@tomjn
Copy link
Contributor

tomjn commented Jan 4, 2020

Describe the bug

In PHP v4, doing this meant copying an object:

$foo  = new Test();
$bar = $foo

Now $foo and $bar are not the same, a copy has been made, and 2 objects exist.

To get around this, WP used references, e.g.:

$foo = new Test();
$boo = &$foo;

Now $bar references $foo, which is the original object. This is why old WP code has &$this when adding object methods to hooks and filters.

This was silly and the PHP engine devs rightly changed this to pass by reference by default in PHP 5, allowing array( $this, 'method' ) to work without lots of duplicate objects being created.

In PHP 5, $foo does not contain the object, but rather a reference to the object, so it can be assigned like normal variables without the bug happening.

Sadly, some of these PHP 4 compat hacks is still in core in the style and scripts system, and have been copy pasted into Gutenberg generating additional PHP warnings in some environments:

Parameter 1 to gutenberg_register_packages_styles() expected to be a reference, value given

Here's the function in question:

/**
 * Registers all the WordPress packages styles that are in the standardized
 * `build/` location.
 *
 * @since 6.7.0

 * @param WP_Styles $styles WP_Styles instance (passed by reference).
 */
function gutenberg_register_packages_styles( &$styles ) {
	// Editor Styles.
	gutenberg_override_style(
		$styles,
		'wp-block-editor',
		gutenberg_url( 'build/block-editor/style.css' ),
		array( 'wp-components', 'wp-editor-font' ),
		filemtime( gutenberg_dir_path() . 'build/editor/style.css' )
	);
	$styles->add_data( 'wp-block-editor', 'rtl', 'replace' );

	gutenberg_override_style(
....

Notice that to avoid duplicating the WP_Styles object in PHP 4, a reference is passed, even though it isn't necessary.

The problem goes away when the ampersand is removed:

function gutenberg_register_packages_styles( $styles ) {

There are numerous other functions in that file that have the same problem and generate the same warning

To reproduce
Steps to reproduce the behavior:

  1. Install Gutenberg and activate
  2. Use it in a PHP environment with Tideways or XHGui
  3. Observe PHP warnings
tomjn added a commit to tomjn/gutenberg that referenced this issue Jan 4, 2020
…re to fix a PHP warning

This closes WordPress#19403 and eliminates a PHP 4.x compatibility fix that causes problems
@aduth aduth added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Code Quality Issues or PRs that relate to code quality labels Jan 6, 2020
@aduth
Copy link
Member

aduth commented Jan 6, 2020

Sadly, some of these PHP 4 compat hacks is still in core in the style and scripts system, and have been copy pasted into Gutenberg generating additional PHP warnings in some environments:

I'm a bit confused about this. It seems to be implied in this issue that prefixing the & in the function arguments is redundant (legacy), but from my reading of the PHP documentation, it's still valid and necessary (at least for function arguments pass-by-reference):

By default, function arguments are passed by value (so that if the value of the argument within the function is changed, it does not get changed outside of the function). To allow a function to modify its arguments, they must be passed by reference.

To have an argument to a function always passed by reference, prepend an ampersand (&) to the argument name in the function definition:

https://www.php.net/manual/en/functions.arguments.php#functions.arguments.by-reference

As to whether Gutenberg needs a reference value, it's unclear. I expect this is carried over from how core handles default script enqueues (possibly what you're referencing):

Could you clarify:

  • Do the warnings occur in a stock installation of Gutenberg?
  • Are they caused by how the $scripts variable is passed around in Gutenberg's internal functions? I would expect that since a wp_default_scripts filter callback is passed with the $scripts reference value, this should not be causing a warning for any of the functions which are defined as callbacks to the wp_default_scripts filter.

@aduth aduth added the [Status] Needs More Info Follow-up required in order to be actionable. label Jan 6, 2020
@tomjn
Copy link
Contributor Author

tomjn commented Jan 21, 2020

I'd say these are carried over from Core, removing them in my local instance eliminates the PHP warnings without impacting the behaviour.

Of note this is also an issue in Core, and there's a Trac ticket for it.

To have an argument to a function always passed by reference, prepend an ampersand (&) to the argument name in the function definition:

If I have the following code:

function makeCall( $obj ) {
    // ....
}
$foo = new Test();
makeCall( $foo );

Then yes, the variable $foo is passed by value, the difference is the value itself. In PHP 5 the value of $foo points to the object, in PHP 4 the value of $foo is the object.

So in PHP 4, it's the object itself that's copied, leading to duplication. In PHP 5, it's the reference that's copied, so it isn't the same reference, but it points to the same object.

This is how object variable assignments work in PHP 5+ by default as a result of how they work with variables, it's non-objects that are copied on assignment such as strings or arrays.

Here's a program to demonstrate, we'll create an object with a counter, and increment it by passing the object into a function:

<?php

class Test {
	public $counter = 1;
}

function increment( Test $obj ) {
    $obj->counter += 1;
}

$foo = new Test();
$foo->counter = 1;
echo $foo->counter . "\n";
increment( $foo );
echo $foo->counter . "\n";
increment( $foo );
echo $foo->counter . "\n";
increment( $foo );
echo $foo->counter . "\n";
increment( $foo );

echo $foo->counter."\n";

Output:

❯ php references.php 
1
2
3
4
5
~/dev 
❯ 

In general it's best to avoid references in modern PHP as they aren't necessary, and because they aren't well understood.

For example, in this PHP code, what do you think the output will be?

$a = 1;
$b = &$a;
$a = 2;
echo $b;

Most people would say 1 but the answer is 2. $b isn't assigned the value of $a, instead it's a reference to $a.

@earnjam
Copy link
Contributor

earnjam commented May 13, 2020

This was fixed in #21987.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Status] Needs More Info Follow-up required in order to be actionable. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
3 participants