View in #core on Slack
@Matt_Robinson: Who is ready for a PR review session?
Main focus, as last week, is:
@Viktor - you raise a few queries about arrays, are you satisfied that single line arrays can stay on one line?
@Viktor: I’m not an expert, your explanation and reasoning is good.
@Matt_Robinson: Happy with he translations as a future project too?
@Viktor: What are you referring to? I may have missed something.
@Matt_Robinson: There were some comments about using
sprintf for translations and that is fine a valid, but we have not fully activated that rule check yet.
@Joy_Reynolds: Keep this PR to the basics, put other stuff on future PRs.
@Viktor: Oh yes. I think it was mentioned that the current set up allows URL changes to localized content… if we had any. For now, I think keeping it as is would be the best option.
@Joy_Reynolds: I have looked at each file. I revisited the ones with changes, but I’m not sure what I’m looking at in some of those. I think a couple of the WP changesets pulled in are out of scope for this PR. Not sure what you want to do with that…
I didn’t try to run it…
@Matt_Robinson: The PR’s pulled in were in the hope of fixing some of the issues raised. I am hoping we are now not too far away from something we can merge.
@Joy_Reynolds: Do you want to go through the remaining conversations?
@Matt_Robinson: I think that’d probably work best.
src/wp-admin/custom-header.php file has a few issues but the entire thing is messy.
@Joy_Reynolds: I don’t see any comments on it, so I assume it’s good.
@Matt_Robinson: I’m looking at them now in the Conversation tab - when it’s overly long the middle section is compressed and must be manually loaded on GH.
If you are happy though I can just resolve them.
@Joy_Reynolds: I was in the Files tab, and I guess I have to expand the diff to see the conversations…
@Matt_Robinson: Does it show in the Files tab if you filter for the specific file?
@Joy_Reynolds: I think all three of those were minor, so if it passes the CS, leave it.
@Matt_Robinson: Next is
The underscored variable use scope - I suspect it is underscored in an attempt to prevent global stomping.
@Joy_Reynolds: I think so too. That’s part of why I prefer it inline. But the whole reason most of this is done is to match WP, so I guess leave it.
@Matt_Robinson: Next is
src/wp-admin/includes/widgets.php - variables set far away from usage
@Joy_Reynolds: Yes, I would prefer to put the ternary into the original, instead of setting another variable 80 lines away.
@Matt_Robinson: Is that more for the
$noform_class - there was a comment about other parameters that are all grouped higher up too.
@Joy_Reynolds: I’m referring to width and height for my comment.
What I don’t know, and shouldn’t have to know, is whether the original variable changes in those 80 lines.
@Matt_Robinson: The variable are currently all collected together in one block. We can consider refactoring the code so that variable are defined immediately prior to use. I suspect it’s a hvit mirrored throughout WP though.
@Joy_Reynolds: I suspect it as well, but we already talked about some changes that were better off as ternaries on one line versus the expanded if statement over several lines. This one adds a variable assignment that is a ternary, so why not just put that into the original? I’m all for getting changes from WP, but not if they are dumb and make things harder.
@Matt_Robinson: So, height, width, an the whole set?
@Joy_Reynolds: I say if it’s just a ternary assignment, moved far away, don’t do that. Put the ternary (with
esc_attr) into the original.
What do you think?
@Matt_Robinson: Looking again, we can only really move the height and width, the other variables are processed to create the invisible widget data for dragging and then the live widget itself if placed. We can do it for those 2 easily though.
@Joy_Reynolds: OK. That’s what my comment was about anyway.
@Matt_Robinson: Shall we do the same - inline ternary for the
$noform_class as well?
@Joy_Reynolds: I think there are a bunch similar which we left, to match WP, so I guess not. They tended to group the PHP instead of put it near where it is used.
@Matt_Robinson: Okay, so for the width the code will be:
<input type="hidden" name="widget-width" class="widget-width" value="<?php echo esc_attr( isset( $control['width'] ) ? $control['width'] : '' ); ?>" />
Similar for the height - updated PR to follow shortly, local testing seems fine.
@Joy_Reynolds: I would put
<input type="hidden" name="widget-width" class="widget-width" value="<?php echo isset( $control['width'] ? esc_attr( $control['width'] ) : '' ); ?>" />`` actually, that's not quite right either. <input type=“hidden” name=“widget-width” class=“widget-width” value="<?php echo isset( $control[‘width’] ) ? esc_attr( $control[‘width’] ) : ‘’; ?>" />``
@Matt_Robinson: Updated PR will follow with that then.
src/wp-admin/setup-config.php with the language direction changes.
@Joy_Reynolds: I’m not seeing a comment.
Oh, that’s because the comment was on the original commit and this is changed?
@Matt_Robinson: I am looking now and wondering if any of that is really needed, it seems CP handles it all well in the
@Joy_Reynolds: What I see is the default value of $dir_attr is empty, so when the page is LTR, there will be no space between attributes because the space was removed.
<ClassicPress/general-template.php at 412e793ca413609ca901adc2a86634172d5b0de1 · ClassicPress/ClassicPress · GitHub | general-template.php>
It looks like it really doesn’t need local handling in addition to the function call.
@Joy_Reynolds: That seems correct to me.
Does it still need the leading space?
I think it does.
@Matt_Robinson: Yes, that needs putting back. I found an early CP commit that made some changes there so we don’t need it, manually reverting.
Next is a couple of long
if blocks in
@Joy_Reynolds: This is similar to the other ones. What do you think?
@Matt_Robinson: They need to be consistent - one excludes the word
style, leave them defined higher up or inline them?
@Joy_Reynolds: I agree if it’s left this way, the string should have the style attribute in it so it doesn’t come out
@Matt_Robinson: Okay, I’ll leave in the current layout but clean up the consistency and spacing.
@Joy_Reynolds: But why are they doing it this way? There is a message telling the user there are no menus, create some. But it is hidden with inline style if there are menus. So is this changed dynamically when a menu is created?
@Matt_Robinson: I don’t know enough about this code to answer that.
@Joy_Reynolds: I mean, the one output when there are menus uses PHP to get the title etc., so it’s not as if it could be done in JS. It just seems like a weird way to do things. I’d rather use a class to hide it, or not output it at all.
@Matt_Robinson: I’ve got to step away for about 15 minutes, will try to come back as I think we are almost there on this PR. And I’m away for the next 2 weeks too.
I have update
$nav_menu_style to include the style declaration now. There do seem to be a lot of unnecessary spaces added in the code as it stands.
@Joy_Reynolds: I was revisiting the Contributing doc and it still doesn’t have anything about working on more than one PR, or testing multiple in a row, so can you tell me if the
git checkout command is safe to use serially? (I think the PRs are all different branches, so it should be, but I don’t want to mess things up or have residual changes from a separate PR).
And before you disappear, can you suggest what can be done to move forward while you are gone?
git checkout will switch branches but you need to take care not to make any local changes as they may well migrate with you as you switch branch. So, it’s fine but you should check
git status or
git diff first to ensure there are no inadvertent changes. If there are deliberate changes then commit them.
@Joy_Reynolds: OK. I think that belongs in the doc.
@Matt_Robinson: While I’m away for the next 2 weeks, any of the PRs in the 1.5.0 milestone can be reviewed:
Either leave a comment in the conversation or the
community reviewed label can be added to the PR - then on my return we should hopefully be able to commit or rebase and commit them a bit faster.
@Laurence: How do you use git @Joy_Reynolds ? Do you use CLI or a GUI?
@Joy_Reynolds: Is there anything else needed for the Layout PR?
@Matt_Robinson: The changes to the
src/wp-admin/nav-menus.php file in the layout PR with regards to links match upstream I think, there were some small layout improvements ready to commit from here.
@Simone_Fioravanti: I use to first create a local branch and then pull multiple PR with git pull.
@Joy_Reynolds: I was going to try to checkout the PR so I could run it to make sure it all works…
@Matt_Robinson: And there are some references to use the core
checked() function, something I’m not against but that will need some manual testing.
@Joy_Reynolds: I thought running those screens that had PRs backported and questions on would be best.
@Matt_Robinson: Agreed, only sometimes tracking down the places in admin where the change applies can be a challenge in itself!
Just 2 files left I think:
src/wp-admin/includes/media.php mentions using checked() - perhaps we can look to pick that up in a later PR
src/wp-admin/nav-menus.php has some long href definitions. The only way to improve those I think is to define a variable first and pass it in later. Or we just leave it.
@Joy_Reynolds: Do you have a preference?
@Matt_Robinson: I think we can look at
checked() later more globally through the code.
The href definitions are counter intuitive really but readable and match upstream, I’d be inclined to leave for now at least.
@Joy_Reynolds: sounds good to me
So if I want to submit a PR for the directory, should I base it on develop or on this PR ?
@Matt_Robinson: Okay, I’ll push the commit I have fixing up what we have discussed. As far as I can tell we have now reviewed all issues raised and either accepted or corrected them.
If the branch can be tested while I’m away then if it all looks like it works the first order of business can be to merge on my return.
I’ve now pushed (hopefully final) changes to the Layout PR. Next meeting I can make is the 17th August - feel free to continue without me though