View in #core on Slack
@Matt_Robinson: Weekly PR review meeting is about to start - who is here?
@Simone_Fioravanti: 
@Matt_Robinson: I’m just having a quick look at #1020 and #1094 recently reviewed, #1020 needs a fix for whitespace at the end of a line.
#1094 can be merge now if we are in agreement. Not in the 1.5.0 milestone but simple to merge.
@Viktor: 1020 doesn’t seem to have a whitespace, at least not that I can see.
line 443
@Matt_Robinson: I’m trying to check that now - seems weird.
@Viktor: Is it possible that it may be related to line endings? I’m on Windows and they are CRLF, and on Linux it’s usually LF.
@Simone_Fioravanti: Don’t see anything strange.
@Matt_Robinson: Got it (I think) - it seems the line number is reported incorrectly - the extra space is on line 428 $exif = array(); - that has a space at the end.
@Viktor: should I submit a fix in my branch?
Nevermind, I see you did it.
@Matt_Robinson: I’ve update the PR, let’s leave the tests to run again.
@Viktor: Unrelated to core PR that can be merged is https://github.com/ClassicPress/ClassicPress/pull/1066
@Matt_Robinson: Everyone happy with #1094?
Has been reviewed by Simone, and I’ll look at 1066 next.
@Laurence: 1066 is good to go. Simple change to readme For $$
@Simone_Fioravanti: Looks good.
@Matt_Robinson: Ah yes, 1066 is the funding.yml file, seems simple - where will we see the results of the change?
@Simone_Fioravanti: Seems that it adds a button in the repo page…
@Matt_Robinson: Certainly reads the way in the docs. I can’t claim I understand it fully but is seems simple enough and certainly won’t break the CP core. I’ll merge it now.
@Viktor: It adds a “Sponsor” button next to “Watch” button, I believe.
Example https://github.com/babel/babel
@Simone_Fioravanti: Works well!
@Viktor: There’s also this:
@Matt_Robinson: It’s merged and visible for CP now - looks good 
@Viktor: My 1022 PR has a conflict from the layout PR. What’s the best way to resolve it?
@Matt_Robinson: You are lucky - just one file!!!
I was looking at #815 this week - 280 conflicting files.
If you want to try I can tell you what I did - are you using app like Github Desktop?
Or the git command line?
@Viktor: I use GitKraken, which is similar to GitHub Desktop. But I can also use CLI.
@Matt_Robinson: Okay so in an app, hopefully you can checkout your PR branch. Then from there there should be a menu option that states “Merge into Current Branch” or similar. That should allow you to select a branch - so that will be develop - you will then hopefully get the merge conflict for that file and it’s a case of working through the conflict in the usual way.
@Simone_Fioravanti: On easy conflicts you can resolve them directly from the browser
@Matt_Robinson: CLI way would be something like:
git checkout your-pr-branch
git fetch upstream
git merge upstream/develop
git push origin your-pr-branch
If you use the browser, I think it shows something currently like this:
<<<<<<< petition2929
$atts = array();
$atts['title'] = ! empty( $item->attr_title ) ? $item->attr_title : '';
$atts['target'] = ! empty( $item->target ) ? $item->target : '';
$atts['rel'] = ! empty( $item->xfn ) ? $item->xfn : '';
$atts['href'] = ! empty( $item->url ) ? $item->url : '';
$atts['aria-current'] = ! empty( $item->current ) ? 'page' : '';
=======
$atts = array();
$atts['title'] = ! empty( $item->attr_title ) ? $item->attr_title : '';
$atts['target'] = ! empty( $item->target ) ? $item->target : '';
$atts['rel'] = ! empty( $item->xfn ) ? $item->xfn : '';
$atts['href'] = ! empty( $item->url ) ? $item->url : '';
>>>>>>> develop
We would need to maintain the bottom of those two layouts.
#1020 is now passing all tests, I’m happy to merge that if we are agreed.
@Simone_Fioravanti:
$atts = array();
$atts['title'] = ! empty( $item->attr_title ) ? $item->attr_title : '';
$atts['target'] = ! empty( $item->target ) ? $item->target : '';
$atts['rel'] = ! empty( $item->xfn ) ? $item->xfn : '';
$atts['href'] = ! empty( $item->url ) ? $item->url : '';
$atts['aria-current'] = ! empty( $item->current ) ? 'page' : '';
Seems you should keep those.
@Matt_Robinson: If you test locally first then, phpcs -n src/wp-includes/class-walker-nav-menu.php - I think the rules say the first = needs aligned but nothing in the ternary statements.
@Simone_Fioravanti: … and fix first and last line indentation.
@Laurence: Which PR are we giving a final check now?
@Viktor: When I ran git fetch upstream it ended with Unlink of file '.git/objects/pack/pack-02d0962fbc4d6ba4ecb135040c2153f6017f09aa.pack' failed. Should I try again? (y/n) Is that normal?
@Matt_Robinson: I’ve never seen that - seems like the .git folder data may be a little corrupt.
@Simone_Fioravanti: Permissions?
ls -lha .git/objects/pack/pack-02d0962fbc4d6ba4ecb135040c2153f6017f09aa.pack
@Laurence: I usually resolve this on the web if it is not so huge a file change.
@Simone_Fioravanti: Maybe you have 2 different Git applications opened?
@Viktor: That could be it, since VS Code is open.
@Matt_Robinson: While Viktor is working some git magic, thoughts on merging 942?
@Laurence: LGTM.
@Matt_Robinson: Done, I also inadvertently pushed a change to SSH upstream and closed #934 by mistake - it’s a good and clean change though, perhaps worth a quick double check by someone else for the sake of transparency.
@Simone_Fioravanti: I’ve received the notice and looked at that, seems a very easy change…
@Matt_Robinson: 
I’m glad someone is keeping on eye on me!
926 and 823 are external library updates with potential impact on PHP8 - that second meeds a conflict fix. Can anyone given an okay to the first while I check out the second?
@Viktor: are you sure it’s 823? that’s a merged PR?
@Matt_Robinson: You are right - 863 - apologies.
@Viktor: I noticed this older PR seems to be completed, is there anything left on it?
https://github.com/ClassicPress/ClassicPress/pull/838
@Matt_Robinson: 838 looks good to merge.
Is 1022 nearer?
Perhaps leave 926 and 863 until next week now as we are already a little over time,
@Laurence: 926 and 863 need time.
838 is good however
@Matt_Robinson: 838 is merged. Some good PRs merged this week - thank you all.
@Simone_Fioravanti: Thank you!