View in #core on Slack
@Matt_Robinson: Hello all, who is here for a PR review meeting?
@Viktor: I’m around
@Joy_Reynolds: Hi
@Matt_Robinson: First step today then, are we satisfied that we can merge the coding standards PR now?
https://github.com/ClassicPress/ClassicPress/pull/1051
@Joy_Reynolds: I think so, although I never got around to running it.
@Matt_Robinson: I run CP on my sites with several PRs merged for my own purposes, I started with the code in 1051, merged in the PRs I use and fixed the conflicts locally and have been using on my sites for about 2 weeks now. I haven’t seen any issues. Merging might allow for broader testing without our small community.
@Joy_Reynolds: Since it touches over 1000 files, I’d hope to have tested it thoroughly, at least opening every admin page, and doing things you don’t normally do to make sure things work.
But it used a tool that is fairly reliable. The questionable parts are the subsequent backports and fixing of merge conflicts.
@Matt_Robinson: We can always leave it a week to allow further time for manual testing.
@Joy_Reynolds: I would say merge and that makes it easier to test. You aren’t releasing it, are you?
@Viktor: Most of the testing by others that don’t participate in core development will most likely be done in RC stage.
@Matt_Robinson: No, I will not be releasing that alone, it contains no improvements for end users. The idea was to merge that and then focus on the 1.5.0 milestone PRs in preparation for a new version RC.
I’m taking the 2x 
to mean proceed with the merge then.
@Joy_Reynolds: Yes
@Viktor: yes
@Matt_Robinson: Merged!
Next then to the 1.5.0 milestone:
https://github.com/ClassicPress/ClassicPress/milestone/16
Is there anything in there we want to get in early? (Provided there are no conflicts with develop now)
@Joy_Reynolds: Oldest first? Or try to do it in the order WP did them in? (sort of)
If you sort by title, a bunch have the commit numbers, so they would be ordered that way.
But I don’t know how to sort this list.
@Viktor: This one was an easy PR and easy/quick to test:
https://github.com/ClassicPress/ClassicPress/pull/1020
@Joy_Reynolds: The milestone contains issues, not PRs?
@Viktor: https://github.com/ClassicPress/ClassicPress/issues/904
@Matt_Robinson: I don’t think we have any community standard of whether milestone should be Issues, PRs or both.
@Joy_Reynolds: OK, how do things get on the milestone?
@Viktor: Usually Matt or Laurence add the label.
This is another easy one, I did test it: https://github.com/ClassicPress/ClassicPress/pull/946
@Joy_Reynolds: If it were me, I would sort the PRs by WP commit number and apply them in that order.
The PHP7 or PHP8 ones need to be done soon.
@Viktor: WP-CLI issue/PR has been tested: https://github.com/ClassicPress/ClassicPress/pull/811
@Matt_Robinson: Pull 1020 seems to need a doc update for some @since blocks.
946 seems good to me, tested, clean to merge now without further work.
@Álvaro_Franz: I’ve also used 1051 while developing a site. So I’m “using” it everyday, for different admin and frontend requests. Had zero issues.
@Matt_Robinson: 811 has conflicts that need fixing before it can be merged.
https://github.com/ClassicPress/ClassicPress/pull/942
This is clean and can be merged without further work.
@Álvaro_Franz: 1083 is one I opened but has errors on tests. Very probably cuz I brought in partial changes from higher WP versions, since I did the backport manually. I want to redo that one with the auto backport script help once layout is done.
@Joy_Reynolds: So, what is the process? Do you comment on it and request the change, or fix it just prior to a merge?
When do you merge? When tests pass? or do you need to run it manually to test it?
@Matt_Robinson: Any that I created I need to look at and figure out the easiest way to process the conflicts, then we can comment on PRs opened by others with the process.
PRs should not really be viable to merge if the tests are not passing. Then it’s a case of reviewing what has changed. If the Pr is relatively simply and perhaps a backport too we can merge with a higher degree of confidence than a large Pr that is not a backport - like the layout PR that we have been reviewing an testing for weeks.
@Joy_Reynolds: OK, now that the Layout PR is in, more of the tests should pass? … or it’s coding standards, not tests?
@Matt_Robinson: Correct, the code layout checks are no more fully enabled - they were running before but were allowed to fail.
You can view test results here:
https://github.com/ClassicPress/ClassicPress/actions
Tests are now complete and all passed from the 1015 merge. Are we happy with 946 being merged now?
@Joy_Reynolds: Seems pretty simple
@Matt_Robinson: https://github.com/ClassicPress/ClassicPress/pull/851
This looks simple but is tagged as needing second review.
And it’s a priority PHP8 fix.
@Joy_Reynolds: What is
> beStrictAboutOutputDuringTests
@Viktor: I have a failing test “Error: Whitespace found at end of line” - how can I see what line is failing?
@Laurence: Apologies to sneak this in. This is for AOB
A number of tests are starting to fail for backport because WordPress has bumped the PHPUnit version and has changed the function names and methods in the tests.
Our next step after 1.5.x “easy” fixes should be to discuss bumping PHPUNIT bump and modernizing the tests. (Should consider also dropping 5.6 if we are to move forward with changes for PHP 8.x.x especially 8.2.
Our manpower will fail us
@Joy_Reynolds: WP has a lot of changesets for all those test changes.
@Matt_Robinson: We already have a PR for PHPUnit 7 and 8
https://github.com/ClassicPress/ClassicPress/pull/815
And it’s in the 1.5.0 milestone
@Joy_Reynolds: Maybe that should be before the others.
But again, do it in the order that WP did.
@Matt_Robinson: The backport are in numerical order for that PR as far as I can tell, just need to resolve the new conflicts now. Maybe we start there next week then.
Thanks all, I have to log off now - until next week.