View in #core on Slack
@Matt_Robinson: Time for the weekly PR review.
As a quick update:
1/ We have the nightly build process functional again
2/ The Renovate process is working again
3/ Several PRs that have been tested have been merged in the last week
4/ Tests are no passing on PHP8
@Joy_Reynolds:
@Matt_Robinson: There are 4 upstream tickets linked here:
https://github.com/ClassicPress/ClassicPress/issues/1098
But on a deeper check, all relate to PHP8.1.
I am proposing today we focus on PHP8 for CP 1.5.0 and PHP8.1 for CP 1.6.0
@Joy_Reynolds: Drawing a line probably gets you closer to release, but they all need to be done.
@Matt_Robinson: Agreed, and that was my thinking. We need to make a release and accept that we will always have more work to do.
So, in the 1.5.0 milestone, we have 3 Issues
and 6 Pull Requests
.
I feel 2 of the Issues
can now be closed.
Setup Automated testing and code sniffs for PHP 8 - this is done. Tests for PHP8 have been running as Experimental for a while and as of this last week are fully passing and no longer classed as Experimental.
PHP 8 Support - this was a summarising Issue and all of the issues and work linked within the comments is now done.
I have recently created:
https://github.com/ClassicPress/ClassicPress/pull/1159
This backports linked to the upstream updates that were WordPress 4.9.21 and 4.9.22 and it also fixes two PHP Warnings I have seen in the admin area.
We will need RC release(s) for 1.5.0 but this is a starting place.
We have an Issue about the petitions widget, I really don’t know what progress has been made on that. If it is close to completion we need the PR, if not we may need to delay that for 1.6.0
@Joy_Reynolds: I know nothing about that.
@Matt_Robinson: I’ll leave a comment on the Issue asking for an update then.
That then leaves 4 backports and the theme support for body-only.
@Joy_Reynolds: The GitHub interface doesn’t show me the milestone link. How do I get there?
Oh, I found it under Issues
Sorry, I usually ignore all that.
@Matt_Robinson: In either the Issues or Pull Requests list there is a filter at the top, over to the right is a link to Labels and then Milestones.
Directly it is:
https://github.com/ClassicPress/ClassicPress/milestone/16
Are you able to update the merge conflict in the theme body-only PR? Were you looking at expanding the tests?
@Joy_Reynolds: I was just looking at that. If I click the Sync link at GitHub, it will discard 19 commits. It doesn’t say what those are though. I don’t know how to see what merge conflicts there are. I thought I handled this already…
It would be great to get PRs looked at in a more timely manner, so they don’t have to keep refreshing. (I don’t like that about Git.)
@Matt_Robinson: I can handle it if easier.
@Joy_Reynolds: That would be great.
@Matt_Robinson: Done and tests are now running. We may need to look at switching assertEquals
to assertSame
for consistency with some of the work done recently in the Unit tests. I recall this is a start and may be possible for extending later. Is what we have now good to merge?
@Joy_Reynolds: I think so, but that’s just me.
@Matt_Robinson: Do you feel you can add anything on the other 4 PRs in the milestone?
@Joy_Reynolds: I was re-reviewing the HTML5 one. (881)
@Matt_Robinson: I’ve been merging that back into code on my sites for months, for themes that declare they are HTML5
it removed a large number of type declarations that are not needed, type="text/css"
and type="text/javascript"
for theme and core enqueued files. It’s not a huge difference but is helpful for those (like me) how like to pass HTML validators.
@Joy_Reynolds: Yes, it is nice to have, but I’m trying to see what was the problem…make sure it’s good to merge.
@Matt_Robinson: The ‘problem’ was that the type attribute was hard-code in a large number of places in the core code. Now there is a conditional check first to see if HTML5 complaint code should be generated.
There was a coding standards issue that’s crept into the theme PR, so while fixing that I’ll update the tests too.
@Joy_Reynolds: There is one place where the conditional CDATA
is output only for non-HTML5. Then another place where it is added hardcoded.
src/wp-includes/widgets/class-wp-widget-archives.php has the added CDATA
The class-wp-widget-categories.php also has it hard coded.
In src/wp-includes/class.wp-scripts.php, new lines were added saying
> // CDATA is not needed for HTML 5.
@Matt_Robinson: Weirdly, this is matching in the backport but it doesn’t seem right either.
@Joy_Reynolds: Yeah, it’s a mixture.
The script-loader.php has it hard coded, but with the comment that it’s not needed on HTML5.
This is in the PR and what I was trying to review:
> This is a second attempt at this enhancement, #524 was reverted in #876 recently due to errors seen with script concatenation.
@Matt_Robinson: The original issue in #524 was a changed variable name from $concat
to $concatenated
where the second was empty when called and therefore broke things. It’s in the script-loader.php
file and in the new version the correct name of $concat
is used.
@Joy_Reynolds: I was just looking at that. It also doesn’t have the chunk
part that I didn’t recognize.
@Matt_Robinson: I’ve updated #872 - I’m happy to merge if you want to have a quick check over one last time.
@Joy_Reynolds: I think the code looks good. If you’ve been running it, I’d say merge.
just those little CDATA
anomalies
@Matt_Robinson: Looking at the current WP core code versus the 4.9.x branch the chunk bit isn’t in 4.9.x either, must have been added in a subsequent release, maybe to handle the Block Editor scripts.
I’ll take a look at the CDATA stuff, seems at least some of it should be conditional now maybe, I think HTML5 ignores it and it’s only there so and XML parser ignores that block of code.
@Joy_Reynolds: I’m not sure it was clear, but I thought the chunk part shouldn’t be in CP, as I think it was added later for block styles.
But the current code splits it into 128 byte chunks instead of using the keys of an array.
@Matt_Robinson: The CDATA segment is commented out in the echo
call and what we have matches WordPress 4.9.x and also the current trunk. I’m not sure we should change without some major testing.
#872 is now merged too.
Shall we get the existing #881 merged too or leave it another week?
@Joy_Reynolds: I think it’s okay
@Matt_Robinson: Okay, I just pushed an update to fix a merge conflict, if it passes I’l merge it in a little while. We are getting there, thanks for your help today, until next week.