View in #core on Slack
@Matt_Robinson: It’s almost time for the weekly PR review meeting, I will be late to the session today, please feel free to make a start without me.
@Simone_Fioravanti: Can we look at #812 that closes #614? It’s a very simple change.
@Joy_Reynolds: The change does a str_replace
, but the issue is “ending with a dot”. Does it matter if there are other dots?
@Simone_Fioravanti: No, the dot is correctly encoded and handled even in the middle of a string.
@Joy_Reynolds: I guess I don’t understand the problem, or why this would fix it.
@Simone_Fioravanti: It’s a bug in osX Mail and other mail client. They automatically recognize a link and make it clickable, but <https://example.org/mypage>.
display like this:
https://example.org/mypage.
Ok, even Slack have the same “bug”.
@ElisabettaC77: That was opened by me. An user name in CP ending in dot (like ELI.CA.) gives errors. And I ended up using ELICA without dots
@Joy_Reynolds: Ah, so it is first rawurlencode
d and then str_replace
all dots, and then parsed inside of network_site_url
. That doesn’t double encode does it?
I mean, the %2e
doesn’t come out %%2e
?
@Matt_Robinson: With the rawurlencode
function happening first (which doesn’t encode hypens, underscores, fullstops / periods or tildes the str_replace
should not result in double encoding.
@Joy_Reynolds: I meant the network_site_url function might encode also.
@Simone_Fioravanti: echo network_site_url( "wp-login.php?action=rp&key=$key&login=" . str_replace( '.', '%2e', rawurlencode( '<http://ELI.CA|ELI.CA>.' ) ), 'login' ) . "\r\n\r\n";
Output: <https://classicpress:8890/wp-login.php?action=rp&key=&login=ELI%2eCA%2e>
@Joy_Reynolds: But the original uses rawurlencode, so it should be okay… looks like it…
@Matt_Robinson: Unless someone applies a filter to network_site_url
then the function doesn’t do any further escaping or encoding.
https://github.com/ClassicPress/ClassicPress/blob/a5170fb3596e7ad57f4e9f2edfc14c84659583ec/src/wp-includes/link-template.php#L3370
Patch seems good to me be something weird happening in the QUnit test - running for 32 minutes, should be faster than that!
@Joy_Reynolds: Is that eating into our quota?
@Matt_Robinson: I think as we are free software we are unlimited. I’ve tried cancelling and running it again.
Are we happy to update the Importer API endpoint? Viktor is working on documentation for WordPress at the moment.
And has anyone tried using current core or nightly on a PHP8 install?
@Simone_Fioravanti: I have now 3 production sites on nightly and PHP8.
The importer API PR looks good to me.
@Joy_Reynolds: I actually have! I spent a day migrating from 20.04 to 22.04 and I chose to install PHP 8.0. My local sites are mostly WP 5.2.x and they all crashed. The stable CP site crashed, but the dev CP site ran fine.
I now have to finagle a multi version installation.
@Matt_Robinson: I did see issues locally with the wp_image_src_get_dimensions
function with large images. Do you have large images to test on these sites?
@Joy_Reynolds: Is there a ticket at WP for that?
@Matt_Robinson: No, it was more of a backport issue I think. Variables not being set.
@Simone_Fioravanti: What you mean with large?
@Matt_Robinson: I’m seeing this:
Notice: Trying to access array offset on value of type bool in /Sites/ClassicPress-fork/src/wp-includes/media.php on line 1345
@Joy_Reynolds: hey, I was in the WP media meeting yesterday and that sort of sounds familiar
@Matt_Robinson: It’s coming from an image added with the WordPress image block data.
@Simone_Fioravanti: Just published: https://educatorecinofilo.dog/hi-classicpress/ with a 5000px image. Seems fine.
@Matt_Robinson: I just overwrote our current function with the one at WP and it all works again.
Source in the post is like this:
<!-- wp:image {"id":906,"align":"center"} -->
<div class="wp-block-image"><figure class="aligncenter"><img src="<https://wpthemetestdata.files.wordpress.com/2013/03/image-alignment-580x300.jpg>" alt="Image Alignment 580x300" class="wp-image-906"/></figure></div>
<!-- /wp:image -->
@Joy_Reynolds: I think this is what was sounding similar #47545: Getting undefined wp.media.frame.state() after click on featured image preview
It isn’t the same thing, but I think that’s what triggered
@Matt_Robinson: Hmm, that seems admin side, I’m seeing issues on the frontend. Maybe because it is a remote image that is linked, there are no image sizes saved and that is where it triggers from though. Something to investigate for next week.
@Joy_Reynolds: It’s also not large
I was reading the changes you made in the contributor.md and it’s a little muddy. Now I don’t install phpUnit, but install composer instead (although it only says that after you need it), is that right? And composer is a global thing or per project? I wasn’t sure what folder I should be in to run the command, either.
@Matt_Robinson: Thanks for the feedback on that, it was rushed as you can probably see.
Composer is / should be a global install.
@Joy_Reynolds: So composer will get phpunit?
@Matt_Robinson: Yes, composer installs all of the files needed in vendor
within the project, so PHPUnit, the Yoast polyfills etc.
You only need to install and run composer if you want to run testing.
It’s a bit like npm
but for php files where npm
is for javascript and node.
@Joy_Reynolds: Does it affect running coding standards?
@Matt_Robinson: It enales phpcs
on project level as an option too yes, so you don’t need WPCS or phpcs
installed elsewhere now either.
@Simone_Fioravanti: I found it very useful.
@Joy_Reynolds: Is there a section in contributing.md for running phpcs
?
I’m still trying to get my new system all set up… I’m having trouble using just the PR test run results, since I can’t find more than 10 coding standard errors on GitHub.
The results page shows the errors, but there are no file names, so you don’t know where to fix anything. The Summary link shows only 10.
@Matt_Robinson: It’s in the tests/phpunit.README.md file. With composer available and after composer install
you can run phpcs
with composer phpcs
from the command line,
@Joy_Reynolds: When you get a chance, I need some thought on my PR to add calls to cp_attributes
, since it standardizes to double quotes and the tests (and perhaps existing plugin filters) check for single quotes in some cases.
@Matt_Robinson: I’ll see if I get time in the next week.
Okay, my time is up, until next week.