Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin: Backport PHP changes for WordPress 6.2 release #47187

Closed
85 tasks done
Mamaduka opened this issue Jan 16, 2023 · 54 comments
Closed
85 tasks done

Plugin: Backport PHP changes for WordPress 6.2 release #47187

Mamaduka opened this issue Jan 16, 2023 · 54 comments
Assignees
Labels
Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.

Comments

@Mamaduka
Copy link
Member

Mamaduka commented Jan 16, 2023

Related Trac ticket 57471.

This is the tracking issue to coordinate the backporting of all PHP changes added in the Gutenberg plugin that needs to be backported for the WordPress 6.2 release.

Files listed for WP 6.2

PRs that are strickenthrough are already merged in WordPress Core

lib

lib/block-supports

lib/compat/wordpress-6.2

lib/experimental

HTML Tag Processor: @ockham, @dmsnell, @adamziel

Getting Involved

If you are interested in helping with the effort, you can comment (or edit the issue's description) with your name next to the file. It would also help to link to Track issues / GitHub PRs in WordPress core when they are available.

Action items when working on backports:

  • Update the files to follow best practices. Mostly rename functions/classes to use the wp_ prefix instead of gutenberg_ and guard with declaration checks for code that needs to be backported to WordPress core.
  • Identify files and functionality that need to be backported to WordPress core.
  • Ensure all files, classes, methods, properties, and functions have the @since 6.2.0 documentation tag included.
  • Ensure all functionality backported to WordPress core has unit tests written.

Issue modeled after #43440

@Mamaduka Mamaduka added Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. labels Jan 16, 2023
@hellofromtonya
Copy link
Contributor

hellofromtonya commented Jan 16, 2023

Hey contributors 👋

I'd like to ask that each backport:

  • be built in smaller PR increments, one thing in a PR
  • open a new Core Trac ticket for each PR
  • build backports as early as possible, i.e. when the thing to backport is ready to go into Core

Why?

  • To ease the pain points and friction of backporting from GB to Core
  • To help Core Committers with code reviews and commits
  • To create a cleaner WP Core SVN commit history
  • To help identify potential issues that may need a follow-up commit or revert
  • To help identify what has and has not been backported, i.e. to hopefully reduce missed backports

@azaozz
Copy link
Contributor

azaozz commented Jan 16, 2023

Like @hellofromtonya I think this method of "syncing" Gutenberg PHP changes to core Trac has proven to be hard to do and error prone. Instead each issue/PR that contains PHP changes and was merged to Gutenberg should be synced separately.

In that terms, thinking this issue should be closed and (maybe) replaced with an issue listing all PRs that contain PHP changes and were merged to Gutenberg. Then each of these PRs would get a corresponding Trac ticket (so there are no omissions) and linked to a patch/another PR where the PHP changes are being synced to https://github.com/WordPress/wordpress-develop.

A good start would be to identify all Gutenberg PRs that contain PHP changes to lib excluding the PRs that add changes to lib/experimental (these should be considered not ready for syncing). If changes from lib/experimental need to be synced to wordpress-develop, they should be moved out of the "experimental" code first, and ideally tested in a Gutenberg release.

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Jan 16, 2023

Backport for wp_theme_has_theme_json() (which includes deprecating WP_Theme_JSON_Resolver::theme_has_support()):

Update Jan 18, 2023:
This backport was committed today and Trac ticket closed ✅

@hellofromtonya
Copy link
Contributor

(Web) Fonts API: I grouped those files together for tracking purposes and self-assigned.

Status: This API is still in development and is not yet ready to backport to Core.

@Mamaduka
Copy link
Member Author

First of all, sorry for the mass ping.

I've updated the PR description with a detailed list1 of PHP changes that need backporting to wordpress-develop in order to be included in WP 6.2. Each PR should get a corresponding Trac ticket and patch (or link to wordpress-develop PR).2

If you're unfamiliar with the process, feel free to ping me (@Mamaduka) or (@ntsekouras). We're happy to help!

Footnotes

  1. PRs changing multiple files are listed multiple times, primarily for tracking purposes.

  2. See the comments from Tonya (https://github.com/WordPress/gutenberg/issues/47187#issuecomment-1384107056) and Andrew (https://github.com/WordPress/gutenberg/issues/47187#issuecomment-1384559863).

@youknowriad
Copy link
Contributor

FYI I'm working on the block patterns categories backports.

@aristath
Copy link
Member

Thank you for the ping @Mamaduka!

border.php - #43375 - @aristath
colors.php - #43375 - @aristath

It appears the 1st change has already been applied to wp-trunk (https://github.com/WordPress/wordpress-develop/blob/0cb8475c0d07d23893b1d73d755eda5f12024585/src/wp-includes/block-supports/border.php#L19), and the 2nd one doesn't need to be backported (code is different on https://github.com/WordPress/wordpress-develop/blob/3dc123624556efbe8953c377e777c12c985ba339/src/wp-includes/block-supports/colors.php#L18 and the change doesn't seem to make sense there)

@andrewserong
Copy link
Contributor

andrewserong commented Jan 17, 2023

Thanks, I'm happy to work on backport PRs for all the ones I've been pinged on (and open up corresponding trac issues). From a quick skim, I think most of mine will depend on the HTML Tag Processor landing first.

I have a backport PR open for a bug fix that has already been approved, and is ready to land, if anyone gets a chance to take a look (I don't think it's listed here because it was a change to the theme JSON class):

@Mamaduka
Copy link
Member Author

Thank you, @andrewserong!

@ockham, @dmsnell, @adamziel, what's the status on HTML Tag Processor? Is it ready for the core?

@ockham
Copy link
Contributor

ockham commented Jan 18, 2023

@ockham, @dmsnell, @adamziel, what's the status on HTML Tag Processor? Is it ready for the core?

I'll discuss with Dennis and Adam today!

@adamziel
Copy link
Contributor

I think it's stable. My only two concerns are PHP 8.2 compatibility (@dmsnell will have more insights) and freezing the API – there is some follow-up work going where @dmsnell and I discussed renaming the processor class. I don't think that's a blocker, though.

@adamziel
Copy link
Contributor

By the way – does anyone know how could we expose the processor as a Composer package directly from the Gutenberg repository? As in – no CI jobs, no composer registry synchronization, just a well-crafted composer.json file that enables non-WordPress folks to use the processor as a dependency in their project.

@youknowriad
Copy link
Contributor

I backported the inert attribute polyfill ( trac ticket https://core.trac.wordpress.org/ticket/57492 ). It's just one part of script-loaded.php changes, so I added a todo list in the issue description for that part.

@ntsekouras
Copy link
Contributor

I opened a PR for #45293 and when this lands I'll backport this one: #45749

Both of them are for Pattern Directory controller.

@andrewserong
Copy link
Contributor

I've made a start on some backporting, and in principle I really like the idea of opening up a trac ticket for each backport we do where we can (I just did one for the position CSS, for example — trac ticket, core PR). I suspect we'll run into a few issues with some features where the JS changes need to be shipped at the same time as the PHP changes, so there might be some backports that will need to wait to land with the package version bump PR. I suppose we can flag those issues as we encounter them?

Next week, I'll look into picking up the fluid typography backport that @ramonjd was pinged on, since he isn't around at the moment.

@ockham
Copy link
Contributor

ockham commented Jan 23, 2023

what's the status on HTML Tag Processor? Is it ready for the core?

I'll discuss with Dennis and Adam today!

I talked to Dennis on Thursday, and he'd rather not include the HTML Tag Processor in 6.2. This means we’ll have to update a bunch of code that’s relying on it right now.

@dmsnell Would you mind taking the lead on this? I can help with it, but since you authored some of those changes, you might be a bit more familiar with them 😊 Since Feature Freeze is on Feb 7, I think we'll need to treat this with relatively hight priority -- it'd be great to get it in within the next 7 days or so so that it can sit a bit before the FF 😄

Edit: I've filed a separate Tracking Issue for this.

@dmsnell
Copy link
Contributor

dmsnell commented Jan 23, 2023

Thanks @ockham for your abundant work on the tag processor already. For context for others, I made a big mistake when expanding the use of the tag processor inside of Gutenberg by not understanding the implications for moving the PHP into Core, as it's been working very well during our rapid development to leave it in the Gutenberg plugin.

Given that mistake, @ockham and I were discussing the merits of leaving it in the Core merge vs. reverting uses of it around Gutenberg. If it pleases you @Mamaduka I'd love to have another couple days to explore and make a final decision. On one hand I hate to revert the uses we already have, as those are now more robust and clear than their earlier implementations, but on the other hand, we've been making high rates of change to the tag processor file and its tests, and the continued development of the HTML parsing subsystem is integral with that module and its internals.

@Mamaduka
Copy link
Member Author

Thank you, @dmsnell!

As long as all necessary parts are migrated into the WP core before the feature freeze, I've no problems with the proposal.

@dmsnell
Copy link
Contributor

dmsnell commented Jan 23, 2023

As long as all necessary parts are migrated into the WP core before the feature freeze

Thanks @Mamaduka - to clarify, I'm trying to figure out which is more prudent: to migrate everything into WP core before the feature freeze; or to revert changes and make sure that nothing from the HTML tag processor ends up in Core this time around.

@ellatrix
Copy link
Member

ellatrix commented Jan 25, 2023

Update: Both are committed ✅

@t-hamano
Copy link
Contributor

t-hamano commented Jan 25, 2023

I made the change in #47398 to load the inert polyfill script into lib/compat/wordpress-6.2/script-loader.php.
Once I submit the ticket and PR to WordPress Core, I will add it to this comment.


@update:
I have submitted a core ticket: https://core.trac.wordpress.org/ticket/57552
PR: WordPress/wordpress-develop#3906


Update: Committed on 1 Feb ✅

@ockham
Copy link
Contributor

ockham commented Jan 25, 2023

Update on the WP_HTML_Tag_Processor situation: We'll be moving forward with backporting that code into Core after all 😊 See #47349 (comment) for more detail.

@hellofromtonya
Copy link
Contributor

Update on the WP_HTML_Tag_Processor situation: We'll be moving forward with backporting that code into Core after all 😊 See #47349 (comment) for more detail.

Hello all 👋 , I've pinged @azaozz for a review of the concerns he raised here, here, and here to ensure those have been mitigated.

@felixarntz
Copy link
Member

@dmsnell @tellthemachines Would it be possible for you to combine some of those use-cases in a single PR based on the branch from WordPress/wordpress-develop#3920?

This would make it easy for us to test the whole thing, which is not really possible with one PR having the API and another one having the use-cases separately.

Eventually we would still merge WordPress/wordpress-develop#3920 first and then could review the use-cases PR separately, but we need to have a way to facilitate testing and performance measurement, hence my advice for now would be to create a new WP core PR with the use-cases, however based on WordPress/wordpress-develop#3920, not trunk.

@youknowriad
Copy link
Contributor

youknowriad commented Feb 1, 2023

@felixarntz I think there's a fundamental problem with how we're doing backports, these PRs and code has already been validated and reviewed in Gutenberg and now folks are being asked to make PRs to validate and review the code again. I think there's something wrong in the process that is slowing us down so much, especially as the beta 1 approaches and that stresses everyone.

The crux of the problem is that Gutenberg is Core, meaning Code that is there should be backportable automatically without too much scrutiny. If folks think that that code is not good enough, we should be adding safe guards in Gutenberg more and have more folks involved in Gutenberg.

Right now, we have a number of backports blocked by that single PR and a few days before beta1. That's not sustainable and unfortunately, it happens for every WP release.

@ntsekouras
Copy link
Contributor

ntsekouras commented Feb 1, 2023

➕ to what @youknowriad said about the process and ways to handle that in the future releases.

This would make it easy for us to test the whole thing, which is not really possible with one PR having the API and another one having the use-cases separately.

@felixarntz for the specific PR: WordPress/wordpress-develop#3920, I think it would be fine to test through the existing tests, as the PR has over 2000 lines of tests. Do you think more tests would be needed?

@felixarntz
Copy link
Member

felixarntz commented Feb 1, 2023

@youknowriad I understand your concern, and I agree this is a problem. But the idea that "Gutenberg is Core" is a bit oversimplified. The development processes and philosophies in Gutenberg are very different from those in Core as of today.

Gutenberg iterations happen a lot more quickly. There is a lot more experimentation going on. With that, also backward compatibility breaks occasionally happen. There is less of a requirement in Gutenberg for at least PHP changes to be accompanied by unit tests than in Core (partly also because certain things are almost impossible to unit tests from the plugin).

I agree with you that we need to find ways to enhance this process, and I see how from your perspective the double review process is painful. But the way Gutenberg and Core have been working is not fully aligned with each other, and until this alignment happens, the double review will naturally happen as well.

We need to get more Core people involved in Gutenberg, and more Gutenberg people involved in Core, and we need to facilitate conversations to improve the workflow issues you're referring to. It goes both ways. I like your idea to add more safe guards in Gutenberg already, but in order to do that in a way that satisfies both sides, we need to have a good number of people involved from both sides. Happy to help where I can.

@felixarntz
Copy link
Member

@youknowriad One more thing regarding your point:

Right now, we have a number of backports blocked by that single PR and a few days before beta1. That's not sustainable and unfortunately, it happens for every WP release.

This particular part of the problem (stress before Beta 1) is something that I believe @hellofromtonya and I have been advocating for for a while now: Gutenberg backports need to happen more regularly to avoid that, it is a problem that it only happens in the "last minute" of a release cycle. Of course it would also become less of a problem if there was less of a need for that double review, but at least where we are today more iterative merges would make it easier (and most importantly less stressful) on that end.

@felixarntz
Copy link
Member

@ntsekouras

for the specific PR: WordPress/wordpress-develop#3920, I think it would be fine to test through the existing tests, as the PR has over 2000 lines of tests. Do you think more tests would be needed?

I was referring to actual testing / QA and performance measurement of the end user experience, not unit tests. The unit test coverage definitely looks comprehensive enough.

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Feb 1, 2023

Update on Fonts API:
It's not ready for WP 6.2. See reasonings here #41479 (comment). I'll remove I removed the following files from the description and tracking:

(Web) Fonts API: Assigned to: @hellofromtonya

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Feb 1, 2023

Right now, we have a number of backports blocked by that single PR and a few days before beta1. That's not sustainable and unfortunately, it happens for every WP release.

@youknowriad I understand your concern, and I agree this is a problem. But the idea that "Gutenberg is Core" is a bit oversimplified. The development processes and philosophies in Gutenberg are very different from those in Core as of today.

I agree with you both @youknowriad and @felixarntz.

In this case, the API isn't fully Core-compatible (yet). Hence the review and changes that are underway.

How could this have been avoided?

  • Backport weeks ago:
    This API has been in Gutenberg since last summer I believe. Once it was known that 6.2 features need it, then it should have been backported then. Had that happened, it would have given plenty of time for folks to do reviews and testing, which would eliminated the bottlenecks and effort happening now this late up against the Beta 1 milestone.

Something being experimented with in 6.2 is: continuous, early, as-soon-as-ready, smaller backports flowing into Core. I hope it roots and grows.

  • Core's CI Jobs running on all Gutenberg PRs.
    That work was completed in Jan 2023, but was after this API was already in Gutenberg.

  • More Core folks actively contributing early and throughout the Gutenberg development cycles.
    Yes, this is needed. Had it happened with this API, then it would have been Core-ready long before the backports.

Of course none of this feedback helps right now to push the API through the review process and get it committed. It's more reflective feedback and food-for-thought to help avoid these types of bottlenecks in the future.

I'd like to imagine a day when the early R&D development and stabilization process within Gutenberg also means the PHP code is already meeting Core commit readiness before a backport starts. When that day arrives, the process will be streamlined.

@dmsnell
Copy link
Contributor

dmsnell commented Feb 1, 2023

@dmsnell @tellthemachines Would it be possible for you to combine some of those use-cases in a single PR based on the branch from WordPress/wordpress-develop#3920?
This would make it easy for us to test the whole thing, which is not really possible with one PR having the API and another one having the use-cases separately.

yes, though I think we're starting to get to the point where we should be acknowledging that these will likely delay the release. I'm personally indifferent to that so I'll do whatever people require. to be clear, you want a PR whose target branch is add/html-tag-processor and which adds some uses of the Tag Processor from Gutenberg into Core?

We ended up reverting and un-reverting #46625 a few times. It's going to live on with a funny legacy.

@felixarntz
Copy link
Member

@dmsnell Yes, I think having something like #46625 in a core PR with base/target branch add/html-tag-processor would be great.

Even better would be to also include what is mentioned in WordPress/wordpress-develop#3920 (comment) if it's straightforward, but I also acknowledge if it takes too much effort, probably okay without and instead do manual review.

@dmsnell
Copy link
Contributor

dmsnell commented Feb 1, 2023

if it's straightforward

As mentioned a few times, none of this is straightforward, else it'd already be incorporated. Even #46625 turned out to be complicated because in order to make a patch there we have to split that Gutenberg PR into two separate PRs in WordPress-devleop, since the settings is new for 6.2.

Unfortunately I just double-checked every use of the Tag Processor and everything except for #46625 was part of a broader change introducing new behavior, which means that there are no PRs in Gutenberg that introduce the use of the Tag Processor unless that's an incidental detail in how they are introducing other new behaviors.

I'm worried that if we start splitting up all this code we're going to make a big headache again for those who want to bring over those features into the merge and they'll have to repeat the surgery I'm attempting to do now to extract parts of their work which never stood on its own.

Still, I created a PR in my fork dmsnell/wordpress-develop#1 which refactors some class-name-mangling logic to rely on the Tag Processor.

@youknowriad
Copy link
Contributor

youknowriad commented Feb 1, 2023

Gutenberg iterations happen a lot more quickly. There is a lot more experimentation going on. With that, also backward compatibility breaks occasionally happen. There is less of a requirement in Gutenberg for at least PHP changes to be accompanied by unit tests than in Core (partly also because certain things are almost impossible to unit tests from the plugin).

If that's really the case, that's not really the intent and have you all more work in Gutenberg might improve things. We shouldn't disregard "Gutenberg is Core" as quickly. Gutenberg packages (JS code) follow the same practices as PHP code and lands in Core without any check. In fact, it's magnitudes more code and more complex code than the php side of things, but it doesn't get as much scrutiny.

The reason is simple, folks that work historically in Core are more familiar with PHP and the code that is in JS can hardly be "reviewed" in "package update" PRs.

What that this say to us: that Gutenberg practices need to efficient enough to cover Core "requirements".

IMO, they are, did we ever see major breakage in Core in the WordPress releases since 5.0 in Gutenberg related code? Not really, some small breaking changes sure and we made huge amount of iterations to our processes to improve things there.

So basically, we should all move away from thinking that "Gutenberg is more iterative so it has less quality" and "backport early" thinking and need more scrutiny when landing to Core. Instead we should all start to think that Gutenberg is part of Core and should have the same level of quality control which means if you all have concerns about code that is in Gutenberg, you should work more on Gutenberg to ensure it has enough quality as soon as it lands.

The only reason "backport early" is suggested is because that allows folks that don't pay attention to Gutenberg to check the code early enough. But that's already too late because the person that did the original PR might not be around anymore to adapt it. So you all need to present from the start.

This is all not a technical issue, it's a trust issue.

@felixarntz
Copy link
Member

felixarntz commented Feb 1, 2023

@youknowriad

We shouldn't disregard "Gutenberg is Core" as quickly.

I am not disregarding that. But I am saying that, as of today, the two are being developed differently to some degree. I want to work towards a future where "Gutenberg is Core" indeed, but this involves increased collaboration between experienced folks from both sides.

Gutenberg packages (JS code) follow the same practices as PHP code and lands in Core without any check.

I'm personally less familiar with the core JS side of things, but based on my familiarity it is safe to say that the Gutenberg standards for writing JS are under way more scrutiny than those for writing JS in Core - which is great.

For PHP though, at least my impression is that the reverse applies. As mentioned in my other comment you're replying to, there is for example more experimentation happening in Gutenberg and there is less PHPUnit test coverage for changes made.

What that this say to us: that Gutenberg practices need to efficient enough to cover Core "requirements".

Exactly. I think there is room for improvement there that we need to collaborate on identifying.

So basically, we should all move away from thinking that "Gutenberg is more iterative so it has less quality" and "backport early" thinking and need more scrutiny when landing to Core. Instead we should all start to think that Gutenberg is part of Core and should have the same level of quality control which means if you all have concerns about code that is in Gutenberg, you should work more on Gutenberg to ensure it has enough quality as soon as it lands.

Yes. But again, it goes both ways. You keep pointing out things that the "classic Core" folks should be doing to improve the situation, but you are not pointing out anything that the Gutenberg folks should be doing. We need to acknowledge that both sides have to learn to collaborate better. Does this mean more "primarily Core" developers should pay attention to Gutenberg? Absolutely! But it also means that "primarily Gutenberg" developers should encourage that and follow up more to proactively implement more steps of the PHP code scrutiny in Gutenberg that has come up in backport PRs to core.

This is all not a technical issue, it's a trust issue.

Again, I think that goes both ways. I wouldn't go as far as calling it a "trust issue", other than that any PR needs review because if you just let it go through, why even have PRs? I see it as a primarily technical issue, as the level of scrutiny (at least for PHP based changes) is currently different in Gutenberg and Core. When a Core developer requests changes on a backport PR, they may not have seen the Gutenberg PR, and that's a natural process in an open source project. And there needs to also be trust on the Gutenberg side that this feedback comes with the best intentions. At least for myself, I can say that I have always been trying to bridge the gap when there seem to be conflicts between two people from both sides, and I would never block a Gutenberg PR with any malicious intent.

As painful as it is, sometimes critical feedback comes late. That is even the case in Core itself, where maybe a critical pushback for some PR that has been open for months only happens 3 days before Beta. I think there is certainly room for improvement on that, but to a degree it's also natural in a decentralized open source project like WordPress.

@youknowriad
Copy link
Contributor

Yes. But again, it goes both ways. You keep pointing out things that the "classic Core" folks should be doing to improve the situation, but you are not pointing out anything that the Gutenberg folks should be doing. We need to acknowledge that both sides have to learn to collaborate better. Does this mean more "primarily Core" developers should pay attention to Gutenberg? Absolutely! But it also means that "primarily Gutenberg" developers should encourage that and follow up more to proactively implement more steps of the PHP code scrutiny in Gutenberg that has come up in backport PRs to core.

I think what you misunderstood in my point here is that in my own opinion, we're already doing what we should be doing to have a great quality in terms of PHP code and follow Core requirements. But we don't know what we don't know so that's where we need more help to improve stuff that we might not be doing. You mentioned php unit tests, I personally think we do enforce these as much as we can. This is was a point that was already raised in previous releases and while I can't speak for everyone, I know I have been paying more attention to.

the level of scrutiny (at least for PHP based changes) is currently different in Gutenberg and Core.

I can't agree with this, I think it's just that it's scrutiny from different people.


What I'm saying is simple.

There's a contributor opening a PR to merge code into Core (Gutenberg here, I use them interchageably), why require two reviews, it should be done at the root level. WordPress and Gutenberg are part of the same process.

For JS code since it's consumed using packages, there's no possibility to do two reviews, so it's defacto "trusted". If PHP code were consumed using composer packages, it would have been the same, we as "WordPress contributors" need to control the quality of our code base and our code base is not just the code that is in Core, it's also the code that is in Gutenberg.

@hellofromtonya
Copy link
Contributor

📢 Reminder 📢

Please add the gutenberg-merge keyword to each of backport Trac ticket. I use this keyword to track what is still open and needing commit review.

Thanks!

@dmsnell
Copy link
Contributor

dmsnell commented Feb 1, 2023

I think it's possible we're getting side-tracked on other issues which might distract from the work at hand. Or at least, maybe we're diverging on solving broader problems than are inherent in the 6.2 merge.

To wit, in the earliest days of the tag processor's design there was active discussion between myself and @azaozz, both of whom have been contributing to Core's PHP for years, and the tag processor has more extensive unit test coverage than most of Core's PHP. @adamziel opened a Trac ticket six months ago in Core (which several Core committers started following) and @gziolo mentioned it in #core-editor to elicit broader feedback. The PR in WordPress-develop has seen rapid and mostly-cosmetic updates in the past day and brings months of reliable in-service vetting in production through the plugin.

So this one fell through the cracks but it's not emblematic of the problems being discussed either. :)

I'll do all I can to help move it through, but I can't address the persistent systematic issues 😄

@felixarntz
Copy link
Member

I think it's safe to say that we are all working towards the same goal, but it's also safe to say that we got some work to do to improve the process. Definitely I hear your pain @dmsnell, I have been in many situations over the years where despite all of the exposure I'm trying to get for a ticket, it is missed until the last minute. That is definitely something I would love to avoid, but is always something that can happen - that's not related to our workflow, but just in the nature of an open source project with contributors that may be more or less active.

@youknowriad

I can't agree with this, I think it's just that it's scrutiny from different people.

That's a fair point, of course this is always a factor. I probably should not have generalized it, it may very well be that it's just different people that have less scrutiny. But that in itself means that a secondary review can make sense. If something was not scrutinized enough when it got merged in Gutenberg, it deserves to receive another look when it gets merged into Core. This would apply the same way in Core though, where we tend to always rely on more than one committer to approve a PR. Actually, this is something I'm curious about, is there a requirement for >1 approvals in Gutenberg?

Regarding PHPUnit tests, that falls in the same area, depends on individual scrutiny of the reviewer(s). The only remaining caveat with Gutenberg is that some changes are actually impossible to unit test in Gutenberg due to their nature, while they could be unit tested in Core. This is not at all related to scrutiny, but just technical limitations. In those situations, we need to pay attention and add test coverage once we can (i.e. in the core PR).

@tellthemachines
Copy link
Contributor

I just put together a PR with all the changes from layout.php that involve WP_HTML_Tag_Processor, based on the tag processor branch. Hope it helps!

@dmsnell
Copy link
Contributor

dmsnell commented Feb 2, 2023

Definitely I hear your pain @dmsnell,

Nope, no pain on my side. I'm just a cog in the machine on this one, and thankfully don't have a foot in the game either way. I was told this code is necessary for other work merging in 6.2 and I had to make quick call whether to rewrite that code, reintroducing bugs that were fixed, or prep the PR for merge, which is what I did after a period of deliberation.

Looks like someone could have marked the Trac ticket as "future-release" or "6.2", but I don't think I'm able and I am largely ignorant of this process, having not been a part of it in the past.

I think it's safe to say that we are all working towards the same goal

That's what makes it great 😄

is there a requirement for >1 approvals in Gutenberg?

Again, a fair point to discuss, but also not relevant for the tag processor, which has been through constant rounds of scrutiny from a variety of developers coming from a variety of places. Almost every line of code in there serves a vetted purpose and stands upon empirical evaluation and experimentation to ensure we reduce the memory footprint and performance impact as much as we practically can (without completely destroying the PHP code). We even went so far as to develop it initially as two competing and independent implementations of the same agreed-upon interface once we had a good idea of what we wanted, to further the rigor with which we were operating. It was built out first from the HTML5 spec instead of from our working intuition of HTML, and the test suite reflects that, being filled with surprising and sometimes bewildering formulations that hardly anyone would expect to find.

My rationale from last July on this, knowing this has the potential to be an extremely critical and hot-path module was:

I think we will want this to be basically as perfect as is plausible as we suggest its inclusion in core.

Or similarly when the merge problem was brought to my attention two weeks ago:

I've never worked with any bit of code I felt had as much criticality or need to get it right before getting it shipped as this. case in point, this is already a decade and a half overdue

So please know that it's with this mindset that I have proposed this HTML parsing system, not because I really want it to get into 6.2, but because I have grown tired of seeing the same old mistakes repeated over and over and finally figured out a way to resolve it once and for all. A merge into 6.2 is an accident; I didn't want it in yet, but the tag processor is solid and given the process mistakes that were made during the release window, it's ready for consideration and in no way lacking review, thought, design, testing, or leading use-cases. It doesn't need to hold up new features or behavioral changes or bug fixes coming over from Gutenberg. (Unless of course someone finds a big stinking problem with it and just flat out rejects it).

Everyone is invited to challenge my feelings towards it and disagree by sharing evidence where it fails. My point in saying all this is that if I didn't believe it met these high standards for inclusion I would not have changed my mind and moved it forward. It is big and it is complicated, but you should find that it meets and exceeds any quality standard applied anywhere in WordPress. If not, I'm happy to chat about discovered concerns.

In the meantime I just want to be helpful and do whatever is required to bring this to a merge, as I know people are anxiously awaiting that. If you are confident that this cannot be merged then it would helpful to know ASAP so we can start rewriting the features introduced since 6.1, as those will require even more review since that would be introducing new unreviewed code at the last minute, and I don't even know how many people would need to be involved because it would be more than a handful. I'm sorry that this is such a mess, and that you, like me, have been pulled in last-minute to ensure a proper and quality release.

This isn't to pressure you into accepting it. I'm just reading all these new comments as they relate to the merge and I know that I am completely unable to resolve the broad and consistent development issues stemming from the WP/GB divide that's being discussed. Maybe I'm just being distracted by thinking that this is what people are wanting fixed before the question of the tag processor can be addressed.

@hellofromtonya
Copy link
Contributor

Hey folks, There's a lot of discussions and amazing context around the HTML API introduction into Core. Request: Can these discussions as well as the contextual information be consolidated into the Trac ticket please? Else I fear this kind of information could be lost or contributors / committers not realize it's happening in here and in Trac/PR.

Also it's hard to follow the backports statuses in this tracking issue.

@felixarntz
Copy link
Member

felixarntz commented Feb 2, 2023

Hey all, I approved WordPress/wordpress-develop#3920, and again, it was never my intention to block it. I also didn't have a particular doubt about that PR, it's just that that PR somehow triggered that "double review" conversation above to start, but it's surely not the only PR affected by that problem. For WordPress/wordpress-develop#3920, we just needed to take the time to get it reviewed and tested, and I'm happy to say from my perspective it's good to go.

On that note, I don't want to derail this conversation here from remaining focused on what is needed for the 6.2 backports, but regarding the workflow and collaboration improvements we've touched on above @youknowriad @dmsnell @hellofromtonya, do you think it's worth opening a Gutenberg issue simply to continue talking about how we can improve things together for the future? It would be great to get to a place where in the future a PR like WordPress/wordpress-develop#3920 can maybe make it through a bit faster.

@dmsnell
Copy link
Contributor

dmsnell commented Feb 2, 2023

@felixarntz I have been trying to communicate just that, that this is unlikely to be a productive place to address the persistent systematic issues 🙃

you're welcome to invite me into such a new issue if you create it but my opinion isn't that important, so please feel no obligation to include me there. we have a… (drumroll) WP_HTML_Processor() in the works that should be ready for 6.3 so we have a chance for a do-over on this 😆 (and I am not creating noise in Trac for that before 6.2 merges - just need to breathe first 😄)

after the investigation so far, it does appear as though those of us building the tag processor followed all the recommended steps for core-submitted PHP, including bringing this to the broader attention of the Core commuters six months ago on Trac and Slack. biggest lesson learned on my end is to make sure someone who is able can mark a Trac ticket as future release at a minimum, and then a specific release when knowable. for that matter I think this particular PR is a kind of extreme one-off. instead of multiple things needing to go wrong in order for this breakdown to occur, many things had to go just right in order to avoid it, and at least one of those things didn't go right. but again, it's not a typical PR.

@andrewserong
Copy link
Contributor

position.php

Now that the tag processor has landed (thanks everyone! ✨), I have a backport PR and trac ticket open for the remaining PHP pieces of the (sticky) position block support feature: Trac Ticket | Core PR

@ntsekouras
Copy link
Contributor

Every back port for beta in this issue has been handled. Any subsequent back port will be handled separately with the standard process(create trac ticket, core PR).

Thank you all for the amazing work and help here! 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.
Projects
None yet
Development

No branches or pull requests