View in #core on Slack
@Matt_Robinson: Weekly Core review time, who is available?
@Simone_Fioravanti: Sorry but I have to go in 30 minutes…
@Matt_Robinson: I’m a bit tied up with work at the moment too. Anything high priority for anyone?
@Simone_Fioravanti: PR 79 so we can get nightly updates. But I see a change request from Viktor.
@ElisabettaC77: Aside for the tests on V2 that is working fine except for what already reported nope. Do we have an ETA for V2?
@Viktor: No set date yet.
@ElisabettaC77: Ok. I think we don’t need to rush it out. Apparently it works fine but better test it very thoroughly to be really sure. This is the release that goes a step further with users leaving WP and it has to work fine.
@Matt_Robinson: @Viktor - I think if you were still seeing an issue with ``Call to undefined function _preload_old_requests_classes_and_interfaces()` then it is either that the function was not added into the local files correctly, or more likely that when you updated it was not in the updated files as they don’t yet contains those changes. Seems reasonable to merge and do more testing and see if it fixes your issues when it is in the nightly build.
@Viktor: I will check. I manually copied files to test.
@Matt_Robinson: The array is the same now as in v1 I think:
https://github.com/ClassicPress/ClassicPress/blob/da86b200c4146e0a6e271788e5a93f2a596bc594/src/wp-admin/includes/update-core.php
Not that it doesn’t need changing though. I can edit locally and push - presume we remove the plugin?
@Viktor: Yes
@Matt_Robinson: CP bundles themes TwentyFifteen through TwentySeventeen so remove the other themes too? Also, I read recently that WP are moving some themes to maintenance only and stopping support for others. I think TwentySeventeen is one they are keeping for now - so maybe remove the other two as well?
@Viktor: Yes, I agree. We may revisit default theme later if we get a chance to create one before v2.0 launches.
@Matt_Robinson: So the $_new_bundled_files array will be just TwentySeventeen - yes?
@EliteStarServices: v2 has been behaving for me also, and I simply download the zip, delete the wp-content folder and dump it on top of what I already have to upgrade (after making a fresh backup).
@ElisabettaC77: About themes, I have seen my v2 install making a strange number even with the CP forks that do support the Customizer by removing it and telling my they were block themes but I have not understood where the issue is or why it does that (the parent themes maybe are the WP ones and are block themes so we need to put in older versions of them?) So I would say let’s bundle a theme for CP and remove the WP default themes from it if we have one theme that can be used. But I will have an opinion about that once I test a bit more with plugins and themes.
@EliteStarServices: WP2019 also works with CPv2
@Matt_Robinson: Update just pushed. I think a bundled CP designed theme sounds like a good thing, I can do themes but not really my skillset, if we can get someone to create a theme we can drop the upstream and child themes and bundle something new and unique.
I’m not quire sure how to remove the review pending on #79 but once tests can run I think I can merge.
@ElisabettaC77: what about Bedrock by Tim? I think for a start it could be a good one?
@Matt_Robinson: @Viktor - #80 looks great, I’ve left a comment about the tests file but for completeness, that file can be left, it is used only in unit testing and is not part of the build. I think we can merge that also.,
@Viktor: Thanks. I published that PR.
@Matt_Robinson: I also made a start on some of the branding changes in installation and Site Health:
https://github.com/ClassicPress/ClassicPress-v2/tree/fix/branding
We should maybe have a look at:
https://github.com/ClassicPress/ClassicPress-v2/issues/76
@Viktor: For Site Health, see my latest comments:
https://github.com/ClassicPress/ClassicPress-v2/issues/76#issuecomment-1561803679
@Matt_Robinson: Okay, #79 is merged. I’ll merge #80 next.
@Viktor: https://github.com/ClassicPress/ClassicPress-v2/pull/73 is a simple addition and looks good. I think it can be merged too.
@Matt_Robinson: Thanks for the comment highlight, I have a change that include the wp_version - I was wondering about adding a header rather than change all instances of wp- to cp- as there are around 70 to change.
@Viktor: If you think that will cause too many merge conflicts later, we can keep them. Maybe we add one main header at the top that says “ClassicPress Troubleshooting Information” or something like that?
@Matt_Robinson: For xample, top of debug clipboard like:
### classicpress ###
platform: 2.0.0+dev (WP-6.2)
### wp-core ###
version: 2.0.0+dev (WP-6.2)
@Viktor: We’re on the same page.
@Matt_Robinson: Does that look okay to you like that?
@Viktor: Let’s use “classicpress site info” text in the heading. That’s what the button says.
Everything else looks good.
@Matt_Robinson: Okay, I’ve just made those changes and also the bit to the “ClassicPress Hosting Team” text, will commit and push now for review.
@Simone_Fioravanti: Sorry but I’m out of time for today 
@Matt_Robinson: Thanks @Simone_Fioravanti
@Viktor: Thanks for participating @Simone_Fioravanti
@Matt_Robinson: @Viktor - I saw the comment left on #68, I hadn’t commented on it before now but I have also been unable to reproduce the issue on the development code and on a nightly install.
@Viktor: It could be me. So I will test it again with new nightly version tomorrow with all these PRs merged. And close issue if I can’t replicate it again.
@Matt_Robinson: I’ve just created a PR for the Site Health Issue.
https://github.com/ClassicPress/ClassicPress-v2/pull/84
@Viktor: Do you want me to test it now or merge and test with nightly?
@Matt_Robinson: Test it later and leave comment on the PR. Fine with me. I have 2 lined up to merge for now.
Anyone anything else?
@Viktor: I just noticed this: https://github.com/ClassicPress/ClassicPress-v2/pull/84/files#diff-cef20a787b7d72d8134219c1e237f97bb3ac8eb52698e32a12b56e23a1060a2cR1377
http:
@Matt_Robinson: That was intentional, if the site can access the CP API, then it can access the outside world, and for privacy reasons we should drop using WP API as much as possible.
@Viktor: Should it be http or https though?
@Matt_Robinson: //Cleans glasses// I see what you mean - I’ll update to https
@Viktor: One other thing from me. Before I open an issue, is our salt keys v1.0 API different from WP’s v1.0? They look different and WP offers v1.1. I see our API says it’s deprecated.
@Matt_Robinson: I think it has been like that for some time, local generation of salt keys is more secure. CP v1.0 offered the same as WP v1.1 though when used.
I wonder if we either deprecate or maybe just flag it more as a warning to potential users.
@Viktor: Many advanced users who can do things in File Manager in cPanel can’t do CLI to generate their own keys. So they do use the API generated ones to copy/paste it. So whatever we do, it still needs to be available.
@ElisabettaC77: https://forums.classicpress.net/t/why-was-the-salts-and-key-generator-deprecated/2104/6 I have found this one back from the old times. It says that during install the installer uses v 1.1 if I am not mistaken, but the wording still says deprecated (for manual installs) - might be something we can word a little better?
@Viktor: Thanks for this. I will check it out.
@Matt_Robinson: In src/wp-admin/setup-config.php we have:
if ( ! $no_api ) {
$secret_keys = wp_remote_get( '<https://api.classicpress.net/secret-key/1.0/salt/>' );
}
if ( $no_api || is_wp_error( $secret_keys ) ) {
$secret_keys = array();
for ( $i = 0; $i < 8; $i++ ) {
$secret_keys[] = wp_generate_password( 64, true, true );
}
} else {
$secret_keys = explode( "\n", wp_remote_retrieve_body( $secret_keys ) );
foreach ( $secret_keys as $k => $v ) {
$secret_keys[ $k ] = substr( $v, 28, 64 );
}
}
I wonder if we just cut out he API code completely and use local generation.
So we are using our deprecated API currently unless there is a $_POST['noapi'] and in that instance $no_api is true and local generation is used, or if there is an API error.
Seems overly complex and maybe could just default to local generation, and leave the API up be deprecated.
@ElisabettaC77: I think that if it can be done we can remove it? Is it a breaking change in the sense that other things might throw errors if removed?
@Viktor: That makes sense. I think dropping API request is good.
@Matt_Robinson: I’ll need to check it out more, it seems that even the API is a fallback:
// Generate keys and salts using secure CSPRNG; fallback to API if enabled; further fallback to original wp_generate_password().
@Viktor: If users who can’t use CLI to generate keys need to change keys, can we add an option for them to regenerate keys locally without CLI? Maybe a constant like repair table or something?
@Matt_Robinson: I have to leave now - thanks everyone.
@Viktor: Thanks Matt. Productive meeting!