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.

What this covers: Adds PHPUnit coverage for enqueue_block_styles_assets() in wp-includes/script-loader.php, which had none. The function has no dedicated test file despite being responsible for enqueuing registered block styles (both via a style_handle and via inline_style), including the on-demand-loading path added for the should_load_block_assets_on_demand filter.

Why these cases: This function was the site of a real regression (#54323): a render_block filter closure was registered with the wrong accepted-arg count, causing an Uncaught ArgumentCountError fatal whenever a block style registered with style_handle rendered on the frontend with on-demand loading enabled. That bug shipped to production and was only caught after the fact (see ticket comment from @jrf: "Why was this not caught be unit tests?"). The five tests added here pin down the exact behaviors the fix depends on:

• style_handle styles are enqueued immediately when on-demand loading is off.
• style_handle styles are deferred and only enqueued once the matching block actually renders when on-demand loading is on (the path that previously fataled).
• style_handle styles are *not* enqueued when a different block renders.
• inline_style styles attach to the default wp-block-library handle when on-demand loading is off.
• inline_style styles attach to the block-specific stylesheet handle instead, when on-demand loading is on and that handle is registered.

Each new test was verified to actually fail when the corresponding logic is broken (including reproducing the original ArgumentCountError by reverting the accepted-arg count), per the usual TDD-style gate for this contribution.

Test-only: This PR only adds test files under tests/phpunit/; it makes no changes to src/. The fix for #54323 already landed in trunk via [51941]/[51981].

Use of AI Tools

AI assistance: Yes
Tool: Claude Code
Used for: Drafting the test cases, verifying each one fails against a deliberately broken implementation and passes against trunk, and writing this description. I reviewed and ran all tests locally before submitting.