WordPress.org

Make WordPress Core

Opened 6 weeks ago

Closed 5 weeks ago

Last modified 5 weeks ago

#54323 closed defect (bug) (fixed)

Too few arguments for function (closure)

Reported by: aristath Owned by: SergeyBiryukov
Milestone: 5.8.2 Priority: normal
Severity: major Version:
Component: Script Loader Keywords: has-patch needs-unit-tests fixed-major
Focuses: coding-standards Cc:

Description

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)

Change History (11)

This ticket was mentioned in PR #1780 on WordPress/wordpress-develop by aristath.


6 weeks ago

  • Keywords has-patch added

#2 @SergeyBiryukov
6 weeks ago

  • Component changed from General to Script Loader
  • Milestone changed from Awaiting Review to 5.8.2

#3 follow-up: @jrf
6 weeks ago

  • Focuses coding-standards added
  • Keywords needs-unit-tests added

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 ?

#4 @SergeyBiryukov
6 weeks ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

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.

#5 @SergeyBiryukov
6 weeks ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to the 5.8 branch.

#6 in reply to: ↑ 3 @SergeyBiryukov
6 weeks ago

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.

  1. 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.

#7 follow-up: @jrf
6 weeks ago

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.

#8 in reply to: ↑ 7 ; follow-up: @SergeyBiryukov
6 weeks ago

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.

#9 in reply to: ↑ 8 @jrf
6 weeks ago

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.

#10 @desrosj
5 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#11 @desrosj
5 weeks ago

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

Note: See TracTickets for help on using tickets.