View in #core on Slack
@Matt_Robinson: Who’s available for a PR meeting?
@Viktor: I’m around, though might not be that much help
@Matt_Robinson: First up should really be a quick recap of the 1.4.2 release. I have only seen positive comments that the release went smoothing - have I missed anything?
@Viktor: No issues reported, at least no in the forums and nothing on Twitter. So we should be good.
@Matt_Robinson: In the API repository then, we have an automatic PR that is created when we make a new release:
https://github.com/ClassicPress/ClassicPress-APIs/pull/48
That change will enable direct migrations for new sites, using the migration plugin, direct to ClassicPress version 1.4.2.
I presume based on the feedback to date we are happy to implement that now.
@Viktor: Sounds good.
@ElisabettaC77: Lurking…
@Matt_Robinson: Okay, I’ll need to commit the cahnge and then update the API. While I’m in that repository there is also a PR for an automated process for WordPress releases too.
@Viktor: Automation is always good, since we don’t have manpower to do stuff manually.
@Matt_Robinson: Currently we declare support specifically rather than assuming migration will be fine - the PR creation will be useful in updating the API and also in reminding or notifying us that a new WP has landed and needs testing.
If anyone wants to see the process steps they are here:
https://github.com/ClassicPress/ClassicPress-APIs/actions/workflows/wordpress-release.yml
@ElisabettaC77: This change is good imho. It will help businesses like mine to convince people that their site can be migrated safely to CP.
@Matt_Robinson: At the very least we can merge it and see what happens when the next version of WP is released. I’ll merge it now.
Okay, that’s done, I’ll update the actual API server later.
Next is in the core itself. I think after the meeting last week PHP compatibility was identified as the main priority for ClassicPress 1.5.0
@Viktor: Yes, we need to focus on PHP 8 as much as possible.
@Matt_Robinson: So, I’d really like to get https://github.com/ClassicPress/ClassicPress/pull/1051 reviewed and merged as soon as possible - the more we merge before this the messier it will get. But it is a HUGE commit.
It’s automated layout fixes to the ClassicPress code - core and tests.
That will enable better testing of future PRs, hopefully easier backporting, and makes the ode more readable.
@Viktor: Why don’t we pause merges until this is merged?
@Matt_Robinson: That’s fine with me, but equally we don’t want to stop development completely.
@Viktor: is there anything we can do to help with it?
@Matt_Robinson: So if we are pausing, then all meetings like this will need to focus on reviewing the PR above until it is done and can be committed.
The changes need checking here:
https://github.com/ClassicPress/ClassicPress/pull/1051/files
@Viktor: Is there anything specific we need to be looking for, like missing curly brackets, etc?
@Matt_Robinson: Essentially it should all be code layout changes. Things like spacing inside brackets / parentheses, ensuring statements like if
and foreach
blocks have curly brackets every time.
@ElisabettaC77: I think in terms of impact. This PR seems a huge undertaking BUT will ease things so has a great impact. I think it is worth to stop merges and focus entirely on it.
@Viktor: Is there a good way for us to track which files were reviewed?
@Matt_Robinson: The one to pay close attention to is the WordPress capitalPDangit check changes some instances of wordpress
to WordPress
- that broke a test and it could also have affected upgrades.
@ElisabettaC77: Ok so, if I open the code in vscode it should tell me if it has “problems” correct?
@Matt_Robinson: I don’t believe that there is a way to track reviewed files unless the reviewer leaves a comment in the Conversation tab, I think @Laurence has made a start and done over 100 files.
@ElisabettaC77: I will have some time after I finish the java course I am attending and will try to review some of the files, if it’s just checking that all brackets are in their place and in pair and things like these I can help.
@Matt_Robinson: VScode I suspect will tell you of any syntax errors so that might be good - it won’t tell you if the layout is correct but then there are GitHub actions that already do that.
@Joy_Reynolds: GitHub has a way to mark which files you have looked at.
@ElisabettaC77: Also reading code will help me review what I learned of PHP during the same java course.
@Matt_Robinson: I just have to pop offline - will be back hopefully in about 15 minutes, need to collect my son.
@Joy_Reynolds: You should see this Viewed
checkbox at the top of each file, so you can keep track.
@Viktor: Also, using VS Code, make sure you’re not using PHP 8 for syntax checks. That will throw extra errors.
@Joy_Reynolds: When I review, I look for what got changed, to see if it changed correctly and if anything was missed.
Each review is per user, so each user has his own checkboxes.
@Viktor: So there’s no way for us to break up work/files between users.
@Joy_Reynolds: Go ahead and check one, and I’ll see if it changes for me…
@Viktor: I’ll do the first one.
@Joy_Reynolds: We don’t want to break it up. We need lots of eyes on everything.
@Viktor: I marked .github/workflows/coding-standards.yml as viewed.
@Joy_Reynolds: still says 0 for me
@Viktor: I found this about “viewed”: https://github.blog/2019-07-01-mark-files-as-viewed/
@Joy_Reynolds: yup
@Viktor: So we need everyone to try and check all 1007 files
@Joy_Reynolds: I don’t know what -n
is for phpcs.
@Viktor: I noticed include() is changed to require(), that’s normal right?
@Joy_Reynolds: depends
@Viktor:
include( ABSPATH . 'wp-admin/admin-header.php' );
require ABSPATH . 'wp-admin/admin-header.php';
@Joy_Reynolds: Is the changeset for removing parentheses from require
part of this PHP compatibility change?
@Viktor: This is not a layout fix, that’s why I’m asking.
@Joy_Reynolds: include
means it can fail and still run. require
means execution stops if it’s not there.
@Matt_Robinson: Sorry, I’m back. I don’t think we all need to check the 1007 files each, but we need a way of collectively checking all of them between us.
@Joy_Reynolds: Matt, are the rules listed in the xml.dist file the ones to check or the exceptions that change the severity?
@Matt_Robinson: Parentheses removal from require
and include
is really only a semantic change as those are commands rather than functions to the parentheses are not necessary.
@Joy_Reynolds: Yes, but it was a different changeset, right?
@Matt_Robinson: The new rules added to the phpcs.xml.dist file are downgraded to warnings from errors, they all still need attention in the future but any changes to fix them will be entriely manual.
I don’t think there should be any changes in that PR that change include
to require
or vice versa
@Viktor: I found at least 2 in one file so far.
@Matt_Robinson: If there are any can you post the full filename with path.
@Joy_Reynolds: mark it in the review?
@Lee_Rickler: @Lee_Rickler has left the channel
@Viktor: Can I add a comment to those changesets? Like this:
@Matt_Robinson: Ah yes, I see one now, in src/wp-admin/about.php
:
include( ABSPATH . 'wp-admin/admin-header.php' );
was changed to:
require ABSPATH . 'wp-admin/admin-header.php';
@Joy_Reynolds: seems like it should be require
@Matt_Robinson: This was not a backport so I suspect there must be a rule for that check somewhere with an automatic fix. It makes some sense as the later code dose need the wp-admin/admin-header.php file to be loaded and include could file resulting in errors on screen.
@Viktor: So what should I do with these? Add a comment when I see it, so you can review?
I need KISS intrustions
@Matt_Robinson: Yes, please add comments for review, we can always say “It’s okay, that’s fine” but worth doing that check.
@Viktor: Should I “add a single comment” or “start a review” when I’m leaving a comment? Refer to the screenshot I sent above.
@Joy_Reynolds: I usually do single comment.
so that it is posted immediately
@Viktor: Thanks. Left one now. I’ll continue review.
@Joy_Reynolds: I looked through a bunch of this stuff when WP did this… Are we using the same rules, but with some as warnings instead of errors?
@Matt_Robinson: It is the same set of rules as WP but this PR is all that could be automatically corrected. The standards are checked with phpcs
but sometimes the rules are written to allow automatic correction too with the phpcbf
command. I have just run the latter command and at the moment we are allowing the other rules to fail as warnings.
@Joy_Reynolds: OK. I commented on the spaces added to square brackets. I thought that wasn’t required.
I could be misremembering, but the second set of brackets didn’t get spaces added.
@Matt_Robinson: I think the rule on spaces in square brackets depends on what is inside the brackets, if it is a variable name for example, it should be spaced.
@Joy_Reynolds: oh, I was not aware of that
@Matt_Robinson: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#space-usage
@Viktor: changes from double quotes to single quotes ok? wp_nonce_url(“x”) to wp_nonce_url(‘x’)
@Joy_Reynolds: I think there is something about using double only where necessary.
@Viktor: Looking at all other code, single quotes are primarily used in everything.
@Joy_Reynolds: > Use single and double quotes when appropriate. If you’re not evaluating anything in the string, use single quotes. You should almost never have to escape quotes in a string, because you can just alternate your quoting style
@Viktor: Should be good in this case: wp_nonce_url( 'comment.php?action='
@Joy_Reynolds: yes, single
@Matt_Robinson: Double quotes are only needed if the string is not completely literal - so it is contains thinks like \n
for a line break, then double quotes are needed.
@Joy_Reynolds: I can’t tell GitHub to ignore spaces, but it thinks it’s a whole new set of lines when a section is indented.
Or maybe ignoring spaces in diff would be better.
@Matt_Robinson: I’ve tracked down the rule for include to require changes it is PEAR.Files.IncludingFile.UseRequire and reports a warning as follows:
File is being unconditionally included; use “require” instead
@Viktor: Is this considered layout change? Part of the code is re-written and new variable added. It’s more than a layout change.
@Joy_Reynolds: The first lines add the check, so it is not inline in the second part.
@Viktor: yes, it’s correct. my question is, should this be happening if we’re only trying to change layout.
@Matt_Robinson: There are two backports with the change because inline if
statements like that are not easy to read and debug. There were about a dozen failing tests after the first run of the autocorrect, and all but one were fixed with 2 backports, see here:
https://github.com/ClassicPress/ClassicPress/pull/1051/commits
So, in short, yes, that is correct.
@Viktor: Thanks. I see it now. Good to know
Is the meeting done? Do we focus all our efforts on getting this PR reviewed?
@Joy_Reynolds: I think so.
@Matt_Robinson: Yes please, I was just looking through some of the comments, but over the next week ideally if we can get those changes reviewed we can commit that change early in next weeks meeting and then start progressing 1.5.0
@Viktor: I’ll see if I can do an announcement to get more people to review file changes. Do you know if regular GitHub users can comment on changsets? Or do they need to be a member of the team/have a role?
@Joy_Reynolds: anyone can comment
@Viktor: OK, good. I’ll see if I can write up some instructions.
@Matt_Robinson: Thank you both. Catch up next week.
@Viktor: If you look at this change, do you see my comment?
https://github.com/ClassicPress/ClassicPress/pull/1051/files#diff-63578939240eff4d3824eeb6a4681ffeb8f5758545dd2ccc251bd4849c5e04d1L512
@Joy_Reynolds: I saw your image where you have the default for diff, but I find it easier to see the line by line changes if I view them side by side. You can mention this is an option.
@Viktor: Oh yes, I forgot about that. That’s a good option.