View in #core on Slack
@Matt_Robinson: Time for the weekly PR review.
Brief update of the week:
API has been updated to include an endpoint for Importers
Several reviewed PRs have been merged into the 1.5.0 milestone
New PRs created to continue PHP compatibility work, now targeting PHP8.1
PR created to make it easer to set up local testing for repository clones
@Simone_Fioravanti: Sorry, I have to leave in 15 minutes
@Matt_Robinson: We have 3 items in the 1.5.0 milestone, 2 added since last week.
The Taxonomy PR seems like it is well tested and can be merged now.
@Simone_Fioravanti: I’ve tested that for days and looks good.
@Joy_Reynolds: I saw the unit test stuff, and it mentioned the contributor’s guide. Will that have any other way but composer?
@Matt_Robinson: Does that mean is there a way, or are we documenting other ways?
@Joy_Reynolds: both
@Matt_Robinson: Yes there are other ways, you can manually install the polyfills and PHPUnit and phpcs
.
IMO we should try to keep things simpler both in terms of execution for anyone wanting to run tests and also so our documentation in the repository does not end up massive and unmanageable.
If someone really can’t use composer
for some reason, the forums are probably the place to discuss alternative setups.
@Joy_Reynolds: I agree with keeping it simple. Where was it that I got the info from in the first place? Maybe James stated how to install phpUnit somewhere? I just don’t have composer, nor the room to install it. I’ve been trying to switch to my 22.04 installation which has more room, but I’ve had video card issues. My 20.04 installation has no room for new stuff.
@Matt_Robinson: I know what you mean - I have 5 versions of PHPUnit on my machine here and PHPUnit 9 is in 2 places.
@Joy_Reynolds: I started making the changes to core files to use the cp_attributes
function (that you merged), but now can’t run phpUnit…
@Matt_Robinson: You can always to create a draft PR in the repository while working on it and let GitHub run all of the tests.
Seems that @Viktor may be online, are you happy with the change to the import api url in the core code?
https://github.com/ClassicPress/ClassicPress/pull/1173
In my testing it seemed like the WordPress API was not being used but that might just be me.
@Viktor: Yes, I left a couple of comments. So far, it’s all good on a production site.
@Matt_Robinson: I see the comments now. The code as it stands points to the latest versions in the WordPress repository so maybe we need to make things better in a future release.
I did wonder if we should remove the WordPress Importer from the API so it isn’t listed.
Also, the api doesn’t serve urls so we may need to figure a way to point to our own plugin directory when thet is functional.
@benlumia007: well currently the importer won’t work and it will fail anyways, maybe just remove it for now until cP has its own release
@Joy_Reynolds: The CP API isn’t pointing to the working version?
@Viktor: For this, I only wanted to introduce the endpoint to replace wp.org endpoint. I clean switch. But we do need a better integration once our directory is live. Removing from the API would be one part of it. Core does have a list hardcoded: https://github.com/ClassicPress/ClassicPress/blob/45d5366ac534bb7a139b40a22e2c56d8eb1e8eeb/src/wp-admin/includes/import.php#L220
<ClassicPress/src/wp-admin/includes/import.php at 45d5366ac534bb7a139b40a22e2c56d8eb1e8eeb · ClassicPress/ClassicPress · GitHub | import.php>
@Matt_Robinson: So while we have a PR affecting the core file should we remove the hard coded entry anyway?
Updating the API endpoint can be done later, all of this is not in production / released code yet.
@Joy_Reynolds: Yes, make it like it should be, while you are changing it.
@Viktor: If we remove WordPress Importer from the list, we should add a link to a doc article providing instructions and an alternative as a temporary solution. People do use it.
@Matt_Robinson: Is there a direct link to the legacy version that works? Or our fork?
@Joy_Reynolds: The importer should be the first thing in the CP Directory.
@Viktor: Latest version of impoter plugin works if installed manually. Last known working version was 0.7, which is what we used to fork ClassicPress Importer (which is close to being finished).
@Matt_Robinson: Okay, let me see if I can have a quick look at that.
What about #1172
Do we already have a link with the manual instructions?
@Viktor: I can write it up if that’s the way we go.
@Joy_Reynolds: Do you want a temporary thing in core? Or just put the temporary thing at the link that core permanently points to?
@Viktor: Importers list pulls data from an array, so there’s no customization we can do to point to another place.
'blogger' => array(
'name' => __( 'Blogger' ),
'description' => __( 'Import posts, comments, and users from a Blogger blog.' ),
'plugin-slug' => 'blogger-importer',
'importer-id' => 'blogger',
),
@Joy_Reynolds: So, change core to expect a link for each?
@Viktor: So do we want to change Importers page structure completely?
@Matt_Robinson: In my quick check we can’t leave the ‘plugin-slug’ empty, the code trued to point to the WordPress plugin directory using the slug, bad UX if the slug is empty - intimates it is installed, and if incorrect will be a broken link.
@Viktor: Adding a notice at the top could work. This page will need to be changed to integrate directory later anyways.
One easy solution can be to simply change WordPress Importer description to note it’s not compatible. This wouldn’t add any new code.
@Matt_Robinson: We aren’t going to fix this one today, I’ll look at the SEOPress code and see how they created their solution. We may be able to simply update the API so point to out WordPress importer fork on GH.
@Joy_Reynolds: Since WP keeps adding block stuff to all their core themes, is there a concern about the ones bundled with CP?
@Matt_Robinson: Can you get me a link to SEOPress plugin - I must have the wrong one I think.
@Joy_Reynolds: What else is needed for a 1.5.0 beta?
@Matt_Robinson: For 1.5.0, this importer stuff needs completed, #1174 perhaps and then we can consider an RC.
@Joy_Reynolds: I don’t see a 1174
@Matt_Robinson: Whoops, #1172
@Joy_Reynolds: Isn’t that a build/test thing, so it doesn’t affect the release?
@Matt_Robinson: Doesn’t impact on the release, you are right. But then such things rarely will so they need reviewing somewhere still, we can merge or defer. But importer will be next week at earliest so perhaps leave it in the milestone for now.
Okay, I have to finish now. More to do with the importer. Until next week.