Automattic / Automattic/nginx-http-concat

PHP Notices raised by cssconcat

Open
#5 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
PHP
Stars
120
Forks
37
PR merge metrics
No merged PRs in 30d

Description

When running unit tests for [Customize Widgets Plus](https://github.com/xwp/wp-customize-widgets-plus) inside of Quickstart, I'm noticing PHP Notices being raised:

```
Only variable references should be returned by reference

/srv/www/wp-content/mu-plugins/http-concat/cssconcat.php:174
/srv/www/wp/wp-includes/class.wp-dependencies.php:219
/srv/www/wp/wp-includes/script-loader.php:653
/srv/www/wp/wp-includes/plugin.php:579
/srv/www/wp/wp-includes/class.wp-styles.php:39
/srv/www/wp-content/mu-plugins/http-concat/cssconcat.php:31
/srv/www/wp-content/mu-plugins/http-concat/cssconcat.php:191
/srv/www/wp/wp-includes/plugin.php:503
/srv/www/wp-tests/tests/phpunit/tests/customize/widgets.php:53
/srv/www/wp-tests/tests/phpunit/tests/customize/widgets.php:175
```

The [code in question](https://github.com/Automattic/vip-quickstart/blob/a13fc5a809f536efa1b186b65689e62944180479/www/wp-content/mu-plugins/http-concat/cssconcat.php#L173-L175) is:

``` php
class WPcom_CSS_Concat extends WP_Styles {
// ...
function &__get( $key ) {
return $this->old_styles->$key;
}
// ...
}
```

I tried changing this method to:

``` php
function &__get( $key ) {
$value = $this->old_styles->$key;
return $value;
}
```

But then of course a different error is raised when `wp_styles()->registered[ $key ]` is attempted to be set:

```
Indirect modification of overloaded property WPcom_CSS_Concat::$registered has no effect

/srv/www/wp/wp-includes/class.wp-dependencies.php:221
/srv/www/wp/wp-includes/functions.wp-styles.php:116
/srv/www/wp-content/plugins/jetpack/_inc/genericons.php:11
/srv/www/wp/wp-includes/plugin.php:503
/srv/www/wp-tests/tests/phpunit/tests/customize/widgets.php:53
/srv/www/wp-tests/tests/phpunit/tests/customize/widgets.php:175
```

I think perhaps the approach for extending `WP_Styles` needs to be changed. Instead of storing the original instance of `WP_Styles` inside of the `WPcom_CSS_Concat` instance, I think it should be fully re-using all of its properties directly.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with www/wp-content/mu-plugins/http-concat/cssconcat.php around lines 173-175 and trace how WPcom_CSS_Concat proxies WP_Styles properties. Compare the failing paths in class.wp-dependencies.php and the customize/widgets.php tests. Done means the PHP notices and indirect-modification error no longer occur while the affected unit tests run.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.