Too few arguments for function (closure)

[aristath]

The issue was reported in the Gutenberg repository: ​https://github.com/WordPress/gutenberg/issues/35912

Registering a block-style when should_load_separate_core_block_assets is enabled throws a fatal error if the block renders on the frontend (more details in the github ticket)

Fixes issue reported in ​https://github.com/WordPress/gutenberg/issues/35912

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

[jrf]

For visibility as it looks like ​PR reviews are not cross-posted:

PR fixes the issue as described. ✅

Two questions/remarks which are not directly related to this PR, but are related to this code snippet:

1. Why is a closure being used here ? Closures can not be "unhooked", so should never be used as a callback function in Core.
2. Why was this not caught be unit tests ? Or to rephrase: might it be a good idea to add some unit tests covering this functionality ?

[SergeyBiryukov]

In 51941:

Script Loader: Correct the number of arguments passed to the closure in enqueue_block_styles_assets().

This avoids an Uncaught ArgumentCountError: Too few arguments to function {closure}(), 1 passed PHP fatal error when registering a block style with the should_load_separate_core_block_assets filter enabled.

Follow-up to [51471].

Props aristath, shimon246, jrf, gziolo.
Fixes #54323.

[SergeyBiryukov]

Reopening for backporting to the 5.8 branch.

[SergeyBiryukov]

Replying to jrf:

Two questions/remarks which are not directly related to this PR, but are related to this code snippet:

1. Why is a closure being used here ? Closures can not be "unhooked", so should never be used as a callback function in Core.

Thanks for bringing this up! Just noting that there was previously some discussion on this in comment:9:ticket:53616. Per @aristath:

This was also mentioned prior to merging the PR, in ​https://github.com/WordPress/wordpress-develop/pull/1482#pullrequestreview-704072508
However, it was deemed acceptable because in this scenario, the anonymous function enqueues a stylesheet and doesn't do anything else:

plugins can still just run wp_dequeue_style to remove the styles. Since they can remove the styles, removing the hook itself seemed inconsequential.

2. Why was this not caught be unit tests ? Or to rephrase: might it be a good idea to add some unit tests covering this functionality ?

It would definitely be a great idea, looks like there are no tests for enqueue_block_styles_assets() yet. The original changesets, [46111] and [51471], did not have any tests either.

[jrf]

Thanks @SergeyBiryukov for providing the historical context!

However, it was deemed acceptable because in this scenario, the anonymous function enqueues a stylesheet and doesn't do anything else...

While the above is true, I still think it should be changed as it may give new(er) contributors the impression that closures as hook callbacks are acceptable. Consistency is key in these things and having a rule like this applied inconsistently only provides confusion and leads to repeated discussions about it (why is it allowed there, but not here ?).

It would definitely be a great idea, looks like there are no tests for enqueue_block_styles_assets() yet. The original changesets, [46111] and [51471], did not have any tests either.

Ouch. That's just painful. Old code not having tests I can understand, we're playing catch-up. But greenfields new code being introduced without tests or with insufficient tests is something I'd really like to eradicate.

[SergeyBiryukov]

Replying to jrf:

However, it was deemed acceptable because in this scenario, the anonymous function enqueues a stylesheet and doesn't do anything else...

While the above is true, I still think it should be changed as it may give new(er) contributors the impression that closures as hook callbacks are acceptable. Consistency is key in these things and having a rule like this applied inconsistently only provides confusion and leads to repeated discussions about it (why is it allowed there, but not here ?).

That makes perfect sense to me :) I agree, let's change this to a proper callback. Worth noting that another instance of a closure as a hook callback was also committed recently in [51902] and would need a similar change.

It would definitely be a great idea, looks like there are no tests for enqueue_block_styles_assets() yet. The original changesets, [46111] and [51471], did not have any tests either.

Ouch. That's just painful. Old code not having tests I can understand, we're playing catch-up. But greenfields new code being introduced without tests or with insufficient tests is something I'd really like to eradicate.

Also agree here. But how do we resolve this? Code like this is merged from Gutenberg. I find the current pace of Gutenberg development a bit too fast to keep up or wrap my head around it enough to write meaningful tests.

[jrf]

Replying to SergeyBiryukov:

Replying to jrf:

While the above is true, I still think it should be changed as it may give new(er) contributors the impression that closures as hook callbacks are acceptable. Consistency is key in these things and having a rule like this applied inconsistently only provides confusion and leads to repeated discussions about it (why is it allowed there, but not here ?).

That makes perfect sense to me :) I agree, let's change this to a proper callback. Worth noting that another instance of a closure as a hook callback was also committed recently in [51902] and would need a similar change.

Yes please!

If I remember correctly, there is an open WPCS ticket to create a sniff to detect this (closures used as hook callbacks). When I find some time, I might just need to make sure that sniff gets written and merged to be included in the next WPCS release. (but 😫, where do I find the time... ?)

Ouch. That's just painful. Old code not having tests I can understand, we're playing catch-up. But greenfields new code being introduced without tests or with insufficient tests is something I'd really like to eradicate.

Also agree here. But how do we resolve this? Code like this is merged from Gutenberg. I find the current pace of Gutenberg development a bit too fast to keep up or wrap my head around it enough to write meaningful tests.

If I'm honest, this is not something for you or me to solve, but something which needs to be addressed in the Gutenberg project. We basically need a firm stance there that no code is allowed to be merged into Gutenberg (or any other new feature sub-projects - fonts API, plugin dependencies API, I'm looking at you!) without tests covering that code.
This policy should apply to both JS code, as well as PHP code.
On top of that, we need a policy for Core that lack of tests/insufficient test code coverage for new code will be considered a non-negotiable blocker for merges into Core.

I suppose we need to put this to the powers that be to get a decision, but the way things are, we're just introducing more and more bugs and technical debt instead of solving anything.

[desrosj]

In 51981:

Script Loader: Correct the number of arguments passed to the closure in enqueue_block_styles_assets().

This avoids an Uncaught ArgumentCountError: Too few arguments to function {closure}(), 1 passed PHP fatal error when registering a block style with the should_load_separate_core_block_assets filter enabled.

Follow-up to [51471].

Props aristath, shimon246, jrf, gziolo, SergeyBiryukov.
Merges [54323] to the 5.8 branch.
Fixes #54323.

[desrosj]

I've created #54367 to handle the removal of the closure.