Make WordPress Core

Opened 3 years ago

Closed 18 months ago

#56865 closed defect (bug) (fixed)

$args taking effect too late in register_block_type_from_metadata

Reported by: aristath's profile aristath Owned by: audrasjb's profile audrasjb
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.2
Component: General Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

The register_block_type_from_metadata function accepts 2 arguments:

  • $file_or_folder
  • $args

The $file_or_folder is a path the the block.json file, or the folder containing it. $args is an array of arguments that can be passed to the function in order to provide complimentary data.
The problem right now is that $args are parsed too late: They get parsed after the scripts/styles have been registered, after all other calculations have been completed, right before we call WP_Block_Type_Registry::get_instance()->register();.
The problem with the current approach is that $args are unable to register script/styles, or even be used to take part in all the processing that the register_block_type_from_metadata function does.
As a side-effect, it's impossible to register a block by passing an array of arguments, without the presence of a block.json file.

Change History (37)

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


3 years ago
#1

  • Keywords has-patch added

#2 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 6.2

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by costdev. View the logs.


2 years ago

#5 @audrasjb
2 years ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

As per today's bug scrub, self assigning for review

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


2 years ago

#7 @mukesh27
2 years ago

  • Keywords needs-testing added

This ticket was discussed in the bug scrub.

THe PR need some manual testing.

Additional props @costdev

#8 @costdev
2 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from 6.2 to 6.3

The PR could also use a unit test to ensure that it's now possible to register a block by passing an array of arguments, without the presence of a block.json file, and is protected going forward.

As this still needs some work, I'm moving this to the 6.3 milestone. If testing and unit tests are ready before RC1, feel free to pull this ticket back into the 6.2 milestone for commit consideration.

@aristath commented on PR #3501:


2 years ago
#9

Added PHPUnit tests and fixed issues that surfaced from said tests.

This ticket was mentioned in Slack in #core by aristath. View the logs.


2 years ago

#11 @costdev
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#12 @oglekler
22 months ago

  • Keywords changes-requested added

@aristath (or anyone who can squeeze it into own to-do list) I was unable to apply the patch, can you please look at it and update, and let's try to make it into 6.3 for not changing milestone continuously.

This ticket was mentioned in PR #4778 on WordPress/wordpress-develop by @felipeelia.


22 months ago
#13

This PR is just an attempt to help with the git conflict of #3501

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

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


22 months ago

@mukesh27 commented on PR #4778:


22 months ago
#15

Thanks @felipeelia, The unit test going fails that need to address.

@felipeelia commented on PR #4778:


22 months ago
#16

I've pushed bd02e2f to address the problem but I'm actually closing the PR, as I see @aristath already fixed the conflict in #3501. Thanks!

@aristath commented on PR #3501:


22 months ago
#17

Please consider https://github.com/WordPress/wordpress-develop/commit/0bfc842f807582906bc119eb62aff6bd1c1e8dea. This change will revert this change.

Can you please elaborate? How does it revert that change? I don't see it 🤔

@spacedmonkey commented on PR #3501:


22 months ago
#18

Can you please elaborate? How does it revert that change? I don't see it 🤔

In https://github.com/WordPress/wordpress-develop/commit/0bfc842f807582906bc119eb62aff6bd1c1e8dea, I made it so the file_exists is not run on the core blocks. The file exists is pointless as it always exists and ends up I/O. In this PR, you move the file_exists out the if statement, make it run again always, reverting my change.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


22 months ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


21 months ago

#21 @audrasjb
21 months ago

  • Milestone changed from 6.3 to 6.4

As per today's bug scrub:
It’s now a bit late for a change like this, and @spacedmonkey requested some other changes in the PR. Let’s move it to 6.4.

This ticket was mentioned in Slack in #core by marybaum. View the logs.


20 months ago

#23 @marybaum
20 months ago

On @jorbin's advice we are leaving this in the very capable hands of the two committers who reported and own this ticket.

#24 @aristath
20 months ago

  • Keywords changes-requested removed

@spacedmonkey commented on PR #3501:


19 months ago
#25

@aristath Is this good to commit.

@aristath commented on PR #3501:


19 months ago
#26

Yes, it is. 👍

#27 @spacedmonkey
19 months ago

  • Keywords commit added

@audrasjb This is good to commit.

#28 follow-up: @audrasjb
19 months ago

Yep thank you for confirming @spacedmonkey, will commit this bugfix next week once I'm back from vacation :)

This ticket was mentioned in Slack in #core by oglekler. View the logs.


19 months ago

#30 in reply to: ↑ 28 @hellofromTonya
19 months ago

Replying to audrasjb:

Yep thank you for confirming @spacedmonkey, will commit this bugfix next week once I'm back from vacation :)

@audrasjb are you planning to commit this ahead of 6.5 Beta 5 (Oct 16th) and/or RC1 (Oct 17th)? Is it ready? Still marked with needs-testing. Not seeing test reports.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


19 months ago

#32 @nicolefurlan
19 months ago

This ticket was discussed in today's bug scrub.

@hellofromTonya is reviewing this. Will commit or punt depending on review.

#33 @hellofromTonya
19 months ago

  • Keywords dev-feedback added

Patch: https://github.com/WordPress/wordpress-develop/pull/3501

Though marked for commit 3 weeks ago (before 6.4 Beta 2), the work was completed and approved this past July.

Given the late stage of the 6.4 cycle with RC1 happening in ~17(ish) hours, I'm not confident enough in that is ready for commit without first shipping in beta releases.

I'm leaning towards punting it to 6.5. But doing a confidence check of those involved.

@costdev @audrasjb @aristath @mukesh27 @spacedmonkey What is your confidence level in the PR based on the above?

If there is high confidence and consensus that this change is ready without (a) side effects or (b) going through beta releases, then it could be considered for commit ahead of RC1.

#34 @hellofromTonya
19 months ago

  • Keywords needs-refresh added; commit dev-feedback removed
  • Milestone changed from 6.4 to 6.5

Chatted in Make/Core slack DM with @costdev who also gave thumbs up to punting. Looking at the patch, there are merged conflicts to resolve. Moving this to 6.5. Also removed the commit keyword until the merge conflicts are resolved. Also noting would be great to get test reports when the patch is ready.

#35 @aristath
19 months ago

  • Keywords commit added

Merge conflicts were resolved, the patch is ready once again.👍

#36 @aristath
19 months ago

  • Keywords needs-testing needs-refresh removed

#37 @SergeyBiryukov
18 months ago

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

In 57026:

Blocks: Parse the arguments earlier in register_block_type_from_metadata().

This makes it possible to register a block by passing an array of arguments, without the presence of a block.json file.

Follow-up to [48141], [49948].

Props aristath, spacedmonkey, mukesh27, costdev, audrasjb, oglekler, felipeelia, hellofromTonya.
Fixes #56865.

Note: See TracTickets for help on using tickets.