Make WordPress Core

Opened 4 years ago

Last modified 2 weeks ago

#53323 assigned enhancement

Place Hello Dolly in containing folder

Reported by: afragen's profile afragen Owned by: whyisjake's profile whyisjake
Milestone: 7.0 Priority: normal
Severity: normal Version: 5.8
Component: Upgrade/Install Keywords:
Focuses: Cc:

Description

Currently Hello Dolly is installed as a single file plugin during a WP core installation. According to Plugin Handbook Best Practices, plugins should be in containing folders. https://developer.wordpress.org/plugins/plugin-basics/best-practices/#folder-structure

This is a simple PR to fix this issue with Hello Dolly. Having this means that things like r51064 are not necessary.

Related #49338

Attachments (4)

updating-helper.diff (4.0 KB) - added by SirLouen 6 months ago.
Core Updating Helper
53323-upgrade-activation.diff (1.1 KB) - added by SirLouen 5 months ago.
53323.diff (12.9 KB) - added by whyisjake 2 months ago.
53323.2.diff (7.7 KB) - added by whyisjake 2 months ago.

Download all attachments as: .zip

Change History (110)

This ticket was mentioned in PR #1330 on WordPress/wordpress-develop by afragen.


4 years ago
#1

Currently Hello Dolly is installed as a single file plugin during a WP core installation. According to Plugin Handbook Best Practices, plugins should be in containing folders. https://developer.wordpress.org/plugins/plugin-basics/best-practices/#folder-structure

This is a simple PR to fix this issue with Hello Dolly. Having this means that things like r51064 are not necessary.

Related #49338

Trac ticket: https://core.trac.wordpress.org/ticket/53323

This ticket was mentioned in Slack in #core by afragen. View the logs.


4 years ago

#3 @afragen
4 years ago

  • Keywords needs-unit-tests added

Looks like it needs an update to some of the unit tests.

#4 @afragen
4 years ago

  • Keywords needs-unit-tests removed

Tests updated

#5 @SergeyBiryukov
4 years ago

Perhaps we should also add hello.php to the $_old_files array in wp-admin/includes/update-core.php, so that those who upgrade from previous WordPress versions don't get two copies of the plugin.

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#6 @afragen
4 years ago

I can do that tomorrow.

#7 @afragen
4 years ago

  • Owner set to afragen

@SergeyBiryukov all recommendations have been added to PR, tests pass 😉

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


4 years ago

#9 @afragen
4 years ago

  • Milestone changed from Awaiting Review to 5.9

#10 follow-up: @afragen
4 years ago

This may actually require the version of Hello Dolly in the plugin repository to be updated to add the header

Text Domain: hello-dolly

This ticket was mentioned in Slack in #core by afragen. View the logs.


4 years ago

#12 @afragen
4 years ago

  • Keywords needs-testing added

In testing, if there is a plugin at hello-dolly/hello.php, updating core, using WP Beta Tester, does not install a new version of hello.php. This should mean that if a user has the older hello.php installed, an update will not over-write the plugin.

Might need more testing, but I think Hello Dolly is only installed in a new installation, not in a update. Therefore anyone who has Hello Dolly installed wouldn't see a change until they updated it, where the plugin repository would install a version in a containing folder.

Last edited 4 years ago by afragen (previous) (diff)

#13 @afragen
4 years ago

  • Milestone changed from 5.9 to 5.8

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-test by pbearne. View the logs.


4 years ago

#17 follow-up: @justinahinon
4 years ago

Thank you for explaining how to test @afragen.

During the test team triage on June 16th, we came through others tests scenarios that might be worth being explored:

  • On an existing installation, updating WordPress Core (with the patch applied) should leave Hello Dolly as a single file
  • On fresh WordPress installation (with the patch applied), Hello Dolly should be in the hello-doly folder

These are the scenarios we are sure about based on the explanations on the ticket.

Now, those that still need to be clarified:

  • What happen when updating the Hello Dolly that's a single file? Should it be left in a single file or moved to the directory?
  • If it is already activated on an existing install, what happens when it's moved? Does it stay activated? Can it cause fatal error then?

Cc @hellofromtonya @boniu91 @pbearne.

#18 in reply to: ↑ 10 @SergeyBiryukov
4 years ago

Replying to afragen:

This may actually require the version of Hello Dolly in the plugin repository to be updated to add the header

Text Domain: hello-dolly

Just noting that Hello Dolly's headers are historically also included in core language files, which will probably be redundant if we move forward with adding the Text Domain header to the plugin.

So if Hello Dolly's translations will be loaded separately as for any other plugin, we might need to remove them from core language files. I don't remember off the top of my head where the code that generates them is located, this will need more investigation.

This ticket was mentioned in Slack in #core-test by engahmeds3ed. View the logs.


4 years ago

#20 @desrosj
4 years ago

  • Milestone changed from 5.8 to 5.9

I'm going to punt this to 5.9 as there are unfortunately too many other tickets that must be addressed in 5.8 and time is running out.

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


4 years ago

#22 @hellofromTonya
4 years ago

  • Keywords reporter-feedback added
  • Milestone changed from 5.9 to Future Release

There are unanswered questions about testing scenarios. Marking for reporter-feedback. It appears to need more discussion and consensus.

Given that 5.9 Feature Freeze is Nov 9 (next week) and this ticket has no activity in the 5.9 cycle, moving it to a Future Release. However, when there is consensus, please move it into a milestone.

This ticket was mentioned in Slack in #core by costdev. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-committers by afragen. View the logs.


3 years ago

#25 in reply to: ↑ 17 @afragen
3 years ago

  • Keywords reporter-feedback removed

Replying to justinahinon:

Sorry this has taken me so long to respond

Thank you for explaining how to test @afragen.

During the test team triage on June 16th, we came through others tests scenarios that might be worth being explored:

  • On an existing installation, updating WordPress Core (with the patch applied) should leave Hello Dolly as a single file

The above is not the current behavior. Currently the single file is replaced with the hello-dolly folder.

  • On fresh WordPress installation (with the patch applied), Hello Dolly should be in the hello-doly folder

Yes.

These are the scenarios we are sure about based on the explanations on the ticket.

Now, those that still need to be clarified:

  • What happen when updating the Hello Dolly that's a single file? Should it be left in a single file or moved to the directory?

An update to a single file Hello Dolly is currently replaced with a version in a containing folder. This does not change.

  • If it is already activated on an existing install, what happens when it's moved? Does it stay activated? Can it cause fatal error then?

Currently an update from a single file Hello Dolly to a containing folder version will update the plugin and deactivate it. This is standard WP behavior. No fatal occurs.

Cc @hellofromtonya @boniu91 @pbearne.

Last edited 3 years ago by afragen (previous) (diff)

#26 @afragen
3 years ago

  • Keywords has-unit-tests added

PR updated. Applies cleanly. Passing tests. Single failure for unable to checkout WordPress/wordpress-develop.

This ticket was mentioned in Slack in #core-test by afragen. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-test by afragen. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-test by afragen. View the logs.


17 months ago

This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.


17 months ago

#31 @afragen
17 months ago

PR has been updated. All tests passing.

#32 @afragen
17 months ago

  • Milestone changed from Future Release to 6.7

This ticket was mentioned in Slack in #core by afragen. View the logs.


16 months ago

#34 @matt
16 months ago

I'm fine with this, thank you!

@peterwilsoncc commented on PR #1330:


15 months ago
#35

This will require an upgrade routine to check if hello.php is in the active list of plugins and replace it with hello-dolly/hello.php. During testing it deactivated the plugin when I checked out this branch.

https://github.com/user-attachments/assets/309a60d0-8e4d-4dee-b366-a04e6f73ed57

For the last few years, Akismet has been automatically updated as part of the build process. This allows for a single source of truth and makes code changes to the plugin easier.

I think the approach may be helpful for Hello Dolly once it's moved in to a sub-folder as it will allow for a single source of truth for the code ratehr than having to update both the plugin and the copy in this repo. It would require a systems request but given there is prior art it should be simple enough to introduce.

@afragen commented on PR #1330:


15 months ago
#36

Does it really matter if the single file hello.php plugin is disabled? If there is ever an update it would also disable.

Since the plugin has no significant effect on the running of the site, I think a seamless transition is overkill.

@afragen commented on PR #1330:


15 months ago
#37

@peterwilsoncc maybe I'm not understanding. If you check in the Playground a new WP installation will have Hello Dolly in a containing folder.

@peterwilsoncc commented on PR #1330:


15 months ago
#38

Given it's possible to have a seamless transition with a few lines of code, I think it's worth the effort.

$active_plugins = get_option( 'active_plugins', array() );
$hello_dolly = array_search( 'hello.php', $active_plugins );
if ( false !== $hello_dolly ) {
        $active_plugins[ $hello_dolly ] = 'hello-dolly/hello.php';
        update_option( 'active_plugins', $active_plugins );
}

I'm not sure what you mean by your second question, are you able to clarify?

#39 @davidbaumwald
15 months ago

Re: translations: I checked with Dion about Sergey's comments in 18 above, and he mentioned that the code itself is in a private repo, but it calls a public CLI command at https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/plugins/wporg-gp-customizations/inc/cli/class-make-core-pot.php?marks=160-168#L133

@afragen commented on PR #1330:


15 months ago
#40

If you have hello.php installed and there's an update, it will install hello-dolly/hello.php. The original plugin would be deleted and the update will be inactive.

@peterwilsoncc commented on PR #1330:


15 months ago
#41

The original plugin would be deleted and the update will be inactive.

@afragen I'm not seeing that, I modified the version number of hello.php and the plugin remained active once it was moved to hello-dolly/hello.php despite the original file being deleted. Maybe there is some snowflake code in core to handle this.

Even though it's a single file plugin, I think WP should handle moving the files as gracefully as it does when other files are relocated. The plugin is active on 700,000+ sites so even when the percentage of sites affected is small, the raw number is large.

@afragen commented on PR #1330:


15 months ago
#42

I yield. Simply giving a counter argument 😉

@peterwilsoncc commented on PR #1330:


15 months ago
#43

Always happy to hear counter arguments. I can be incredibly wrong sometimes.

I'll check with meta (@dd32, I mean @dd32) about using an external to see if we can get a single source of truth for the code.

#44 @peterwilsoncc
14 months ago

To summarize the discussion on the PR:

  • there will need to be some code to prevent the relocation of the plugin from causing it to be deactivated. The plugin has 700K+ active installs.
  • the upgrade routine runs too late to modify the active plugins list, by the time it runs the plugin will have been deactivated and the opportunity missed
  • to avoid the need to maintain the plugin in two locations, the hello-dolly folder can be added as an external and added during the build process similar to Akismet. I've started discussions with the meta team about the process here

If needs be it might be possible to add some snowflake code to wp_get_active_and_valid_plugins() to account for the relocation but it would be optimal if that could be avoided.

There is some snowflake code (either in WP or the upgrade API) to keep the plugin active when updated via the dashboard so there might be some prior art that can used.

#45 @davidbaumwald
13 months ago

  • Milestone changed from 6.7 to 6.8

There's still some open #meta questions and considerations prior to completing this in Core.
6.8Future Release given a lack of recent momentum towards a resolution.

If any committer feels the remaining work can be resolved in time for Beta 1 and wishes to assume ownership, feel free to update the milestone accordingly.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


9 months ago

#47 @audrasjb
9 months ago

  • Milestone changed from 6.8 to Future Release

As per today's bugscrub: since the Meta issues are very unlikely to be resolved in time for 6.8, let's move this issue to Future Release.

#48 @SirLouen
6 months ago

  • Keywords needs-refresh added

@afragen currently, this PR 1330 is not applying cleanly in 6.9-alpha-60093-src. Could you give it a check for further testing?

#49 @afragen
6 months ago

Will do.

@afragen commented on PR #1330:


6 months ago
#50

Clean merge and passing tests.

#51 @afragen
6 months ago

@SirLouen I think I've gotten everything refreshed. It clean merge and passing tests.

@SirLouen
6 months ago

Core Updating Helper

#52 @SirLouen
6 months ago

  • Keywords has-test-info added; needs-testing removed

Test Report

Description

❌ This report can't validate that the indicated patch works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/1330.diff

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 137.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Core Updater Mod 1.0.0
    • Test Reports 1.2.0

Testing Protocol

I'm going to explain step by step how I have tested this patch. It's a little tricky, so I will provide the maximum files and instructions to help in the selection of the best method:

  1. First, we need the WordPress update that will come with the patch already deployed. Two methods
  1. Second, we need to edit a little the standard core update protocol, so it can accept our unofficial file. Apply this patch I have attached
  1. We need to add some code to use the added filters in a plugin, functions.php or wherever it can be executed. I attach the code in the Supplemental Artifacts: here we specify which folder are we using to put our wp-update.zip file. You can set whatever path you prefer, just make sure it has full permissions from the web server.
  1. Now we add the wp-update.zip file to the folder we have set in our previous step
  1. We go to plugins and enable Hello Dolly
  1. We go to Dashboard > Updates and execute the core update (in wordpress-develop there is always an update available; otherwise, we will need to go to wp-include/version.php and edit the $wp_version to have a lesser version to trigger the update.
  1. If everything has been done correctly, after the update you will see something like:

https://i.imgur.com/sH8obGE.png

Expected Results

  • After the update, we expect that Hello Dolly will be in the new directory format, and it's still enabled.

Actual Results

  1. ❌ Hello Dolly disappears after the update

Additional Notes

  • @afragen it seems that the patch is not working, I have not reviewed the code because I was expecting that you already did in the past with @peterwilsoncc, and it was ready to be shipped. Please review my steps to check if I'm doing something wrongly, or check the code to see if something has broken in the meantime.

Supplemental Artifacts

add_filter( 'custom_update_package', 'use_local_update_zip', 10, 3 );
function use_local_update_zip( $package_url, $to_download, $current ) {
    $local_zip_path = ABSPATH . '../update/wp-update.zip';

    if ( file_exists( $local_zip_path ) ) {
        return $local_zip_path;
    }
    return $package_url;
}

add_filter( 'check_signature_for_package', 'skip_signature_for_local_zip', 10, 4 );
function skip_signature_for_local_zip( $check_signatures, $package_url, $to_download, $current ) {
    return false;
}

add_filter( 'pre_check_md5_for_custom_package', 'skip_md5_check_for_local_zip', 10, 3 );
function skip_md5_check_for_local_zip( $perform_check, $wp_version, $locale ) {
    return false;
}

#53 @afragen
6 months ago

  • Keywords needs-refresh has-patch removed

The nightly doesn't install Hello Dolly or Akismet. Only a beta/RC/release will install it.

It's unlikely that Hello Dolly will remain active due to what happens in core when the plugin file is moved.

Last edited 6 months ago by afragen (previous) (diff)

#54 follow-up: @afragen
6 months ago

If you test using https://playground.wordpress.net/wordpress.html?pr=1330 you will see that Hello Dolly is installed correctly in the containing folder.

#55 in reply to: ↑ 54 @SirLouen
6 months ago

  • Keywords changes-requested has-patch added

Replying to afragen:

If you test using https://playground.wordpress.net/wordpress.html?pr=1330 you will see that Hello Dolly is installed correctly in the containing folder.

I'm not convinced that this is the right test. AFAIK, calling playground with a PR, just patches the current version of PG with the files, hence it will appear logically there.

The nightly doesn't install Hello Dolly or Akismet. Only a beta/RC/release will install it.

I've added a new wp-update.zip (same one as in the testing info), I have faked it to version 6.8.2, so it's a final release technically now.

https://i.imgur.com/FpV9yu4.png

But still the plugin doesnt appear.

Last edited 6 months ago by SirLouen (previous) (diff)

#56 @SirLouen
5 months ago

@afragen another method I have recently discovered to update a custom build, is by using wp core update wp-update.zip with wp-cli. I've been breaking my head with the wall to achieve this kind of core updating for ages.

But still the latest patch is not working as in last report.

I have to start using CLI more, it's definitely packed with surprises

#57 follow-up: @afragen
5 months ago

@SirLouen just out of curiosity do you think the changes in wp-admin/includes/update.core.php are causing this test failure?

#58 in reply to: ↑ 57 @SirLouen
5 months ago

Replying to afragen:

@SirLouen just out of curiosity do you think the changes in wp-admin/includes/update.core.php are causing this test failure?

I have not reviewed the code, just set the testing protocol, but I'm not really sure why the plugin is completely disappearing (akismet is there). Maybe we should take a look to how things are being currently done with Akismet to do something similar?

By the way, here my new steps with wordpress-develop (also a reminder for me if I have to retest, because I'm testing so many reports lately, that now I'm starting to mix memories)

  1. Get the patch and modify the version of WordPress to 6.8.2 in version.php
  2. npm run build
  3. mv build/ wordpress/
  4. zip -r wp-update.zip wordpress/
  5. rm -r wordpress/
  6. Clean all changes to trunk (git restore .)
  7. Go into WP and enable Hello Dolly plugin
  8. npm run env:cli core update wp-update.zip
  9. rm wp-update.zip
  10. Check results (Hello Dolly disappearing)

#59 follow-up: @afragen
5 months ago

The code referenced above specifically removes hello.php.

#60 in reply to: ↑ 59 @SirLouen
5 months ago

Replying to afragen:

The code referenced above specifically removes hello.php.

I think I've found why it's not bringing the plugin back:

In this line you should be adding

'plugins/hello-dolly/'      => '6.9',

But still there is one problem to solve: if hello.php was active before update, it should remind active, but with the current pach is not.

This is something to be updated in the database during the upgrade.

For example in this line

// Switch Hello Dolly from file to directory format. See #53323
_upgrade_690_update_active_hello_dolly_option();

And then by the end of the file:

function _upgrade_690_update_active_hello_dolly_option() {
    $active_plugins = get_option('active_plugins');
    $old_plugin = 'hello.php';
    $new_plugin = 'hello-dolly/hello.php';
        $key = array_search($old_plugin, $active_plugins);
    
    if ( $key ) {
        $active_plugins[$key] = $new_plugin;
        update_option('active_plugins', $active_plugins);
    }
}

#61 follow-up: @afragen
5 months ago

@SirLouen great sleuthing. I didn’t see the array to add the directory. I’ll update the PR.

My suggestion is to create another PR to keep Hello Dolly active. Just my opinion but if deactivated I doubt the site owner will notice. After all, it’s only present to show the “hope and enthusiasm of a generation”.

#62 @afragen
5 months ago

@SirLouen PR updated

#63 in reply to: ↑ 61 @SirLouen
5 months ago

Replying to afragen:

@SirLouen great sleuthing. I didn’t see the array to add the directory. I’ll update the PR.

My suggestion is to create another PR to keep Hello Dolly active. Just my opinion but if deactivated I doubt the site owner will notice. After all, it’s only present to show the “hope and enthusiasm of a generation”.

This is something that was commented by @peterwilsoncc back in the day. It has so many active installs (still 700,000+ nowadays)

More info about my research: I've tested and its not working with the code I provided. This seems that is going to be tricky as hell. Let's see how can I stepdebug a core update run with wp-cli…

Maybe we could open another PR for this, but both should go in at the same time; otherwise the second will be completely useless since it's so specific to this topic. I agree that is nothing massive to keep it up after update (and it feels its going to be a hassle, maybe @peterwilsoncc could give some ideas on how to make this happen based on my last comment)

Also very important don't forget to add the rules in .gitignore for this patch, to avoid deletion of hello for the repository (it's like the default plugin and should be tracked).

https://i.imgur.com/CQrj9rq.png

#64 follow-up: @afragen
5 months ago

Hmm, it's never been in the .gitignore before, only the whole plugins directory.

#65 in reply to: ↑ 64 @SirLouen
5 months ago

Replying to afragen:

Hmm, it's never been in the .gitignore before, only the whole plugins directory.

That screen capture is what needs to be modified, full plugins ignored, only new hello-dolly directory not ignored (because now its falling into the plugins directory, hence, its going to be ignored by default)

#67 @SirLouen
5 months ago

I decided to step debug using my previous upgrade technique, because I've been unable to run Xdebug with WP-Cli 💀

Finally, I found the problem of why my solution despite the code was right (I tested the code independently), wasn't applying during the core upgrade.

It appears that placing this code in that file proposed will only apply when updating via the Update button in /wp-admin/update-core.php but not via wp-cli core update because it happens that CLI is taking another route for upgrading (and maybe automated upgrades also take another route, who knows 🤷‍♂️)

I'm not an expert in Core Upgrades area but from what I've been reviewing a possible solution that is working for me is adding the code in src/wp-admin/includes/upgrade.php (attached patch here). Now it works with CLI, with my hacked core upgrading system, and probably, with automated updates also, because it seems that all upgrading skins use this upgrade.php file to do their diligence.

And with this finally accomplished, I would suggest that this patch is ready to go.

Last edited 5 months ago by SirLouen (previous) (diff)

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


4 months ago

#69 follow-up: @oglekler
4 months ago

Related tickets were discussed during the Test team triage session.

We have several tickets about Hello Dolly, and from my point of view they have valid points and can be incorporated into one patch, so I am linking them for further discussion.
#63621, #61215, #61069, #58363

#70 in reply to: ↑ 69 ; follow-up: @afragen
4 months ago

Replying to oglekler:

Related tickets were discussed during the Test team triage session.

We have several tickets about Hello Dolly, and from my point of view they have valid points and can be incorporated into one patch, so I am linking them for further discussion.
#63621, #61215, #61069, #58363

I disagree. This ticket is not about fixing or changing the content of the Hello Dolly plugin or its relationship to the PCP. It's simply about making the plugin conform to the standard of having a containing folder.

Last edited 4 months ago by afragen (previous) (diff)

#71 in reply to: ↑ 70 @SirLouen
4 months ago

Replying to afragen:

I disagree. This ticket is not about fixing or changing the content of the Hello Dolly plugin or it's relationship to the PCP.

By the way, shouldn't we milestone it to 6.9?

#72 @afragen
4 months ago

  • Milestone changed from Future Release to 6.9

@whyisjake
2 months ago

#73 @whyisjake
2 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 60666:

Upgrade/Install: Move Hello Dolly plugin to directory structure.

Changes the Hello Dolly plugin from a single file structure to a proper plugin directory structure, moving from hello.php to hello-dolly/hello.php to align with Plugin Handbook Best Practices.

  • Adds proper Text Domain: hello-dolly header to Hello Dolly plugin
  • Updates core files to remove special case handling for hello.php
  • Updates plugin dependency system to handle new directory structure
  • Adds upgrade routine to migrate active plugin references and keep plugin active
  • Updates all tests to use new plugin path format hello-dolly/hello.php
  • Updates build configuration and .gitignore for new directory structure
  • Adds hello.php to old files list for cleanup during core updates
  • Adds plugins/hello-dolly/ to new bundled directories list

Props afragen, SergeyBiryukov, peterwilsoncc, SirLouen, matt, davidbaumwald, desrosj, hellofromtonya, justinahinon,audrasjb, oglekler, whyisjake.
Fixes #53323.

#74 @whyisjake
2 months ago

In 60670:

Upgrade/Install: Actually move Hello Dolly plugin to directory structure.

This fixes an issue from [60666] where I neglected to svn move the files.

Follow-up to [60666].

Reviewed by jeremyfelt.

Props swissspidy.

See #53323.

#75 @johnbillion
2 months ago

  • Keywords needs-unit-tests added; has-unit-tests has-test-info changes-requested has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

There are still test errors and failures in trunk. Example failing workflow: https://github.com/WordPress/wordpress-develop/actions/runs/17250411316/job/48952910236.

There were 2 errors:

1) Tests_Admin_IncludesPlugin::test_get_plugin_data
file_get_contents(/var/www/tests/phpunit/includes/../data/plugins/hello-dolly/hello.php): failed to open stream: No such file or directory

/var/www/src/wp-includes/functions.php:6901
/var/www/src/wp-admin/includes/plugin.php:94
/var/www/tests/phpunit/tests/admin/includesPlugin.php:25
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97
/var/www/vendor/bin/phpunit:118

2) Tests_Admin_IncludesPlugin::test_is_network_only_plugin_hello
file_get_contents(/var/www/tests/phpunit/data/plugins/hello-dolly/hello.php): failed to open stream: No such file or directory

/var/www/src/wp-includes/functions.php:6901
/var/www/src/wp-admin/includes/plugin.php:94
/var/www/src/wp-admin/includes/plugin.php:604
/var/www/tests/phpunit/tests/admin/includesPlugin.php:553
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97
/var/www/vendor/bin/phpunit:118

--

There were 4 failures:

1) Tests_Admin_IncludesPlugin::test_is_plugin_active_true
Failed asserting that false is true.

/var/www/tests/phpunit/tests/admin/includesPlugin.php:379
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97
/var/www/vendor/bin/phpunit:118

2) Tests_Admin_IncludesPlugin::test_is_plugin_inactive_false
Failed asserting that true is false.

/var/www/tests/phpunit/tests/admin/includesPlugin.php:399
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97
/var/www/vendor/bin/phpunit:118

3) Tests_Admin_IncludesPlugin::test_activate_plugins_single_no_array
Failed asserting that false is true.

/var/www/tests/phpunit/tests/admin/includesPlugin.php:575
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97
/var/www/vendor/bin/phpunit:118

4) Tests_Admin_IncludesPlugin::test_activate_plugins_single_array
Failed asserting that false is true.

/var/www/tests/phpunit/tests/admin/includesPlugin.php:585
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97
/var/www/vendor/bin/phpunit:118

#76 @johnbillion
2 months ago

  • Owner changed from afragen to whyisjake
  • Status changed from reopened to assigned

#77 @whyisjake
2 months ago

[60675] fixed the svn:ignore props and [60677] fixed the broken tests.

@whyisjake
2 months ago

#78 follow-up: @whyisjake
2 months ago

  • Keywords has-patch has-unit-tests reporter-feedback added; needs-unit-tests removed

After chatting with @peterwilsoncc and @afragen, I think we need to be a little more explicit in the fallback handling here. In 53323.diff I added a filter to the wp_get_active_and_valid_plugins() function to ensure that we get the correct version if active.

Tests attached.

#79 in reply to: ↑ 78 @SirLouen
2 months ago

Replying to whyisjake:

After chatting with @peterwilsoncc and @afragen, I think we need to be a little more explicit in the fallback handling here. In 53323.diff I added a filter to the wp_get_active_and_valid_plugins() function to ensure that we get the correct version if active.

I have mixed feelings with this update specially because its a single-use hook that is going to added to the hook stack forever.

Also, @peterwilsoncc can you review the tests? I feel that some setups or tear-downs could be useful here, what do you think?

Last edited 2 months ago by SirLouen (previous) (diff)

#80 follow-up: @johnbillion
2 months ago

  • I would rather we didn't introduce logic that runs on every request to handle the Hello Dolly migration.
  • What's the purpose of the wp_should_migrate_hello_dolly filter?
  • Can we get PRs instead of patches too please? So the test suite can run.

This ticket was mentioned in PR #9642 on WordPress/wordpress-develop by @whyisjake.


2 months ago
#81

Adds some explicit migration handling for the hello dolly plugin.

#82 @mindctrl
2 months ago

I agree with @johnbillion about this not running on every request. Could this be handled in an upgrade routine instead (wp-admin/includes/upgrade.php)?

#83 @afragen
2 months ago

Is there a way to run a query to determine how many sites are running the single file plugin vs the version in a containing folder?

That data would be interesting.

#84 in reply to: ↑ 80 ; follow-up: @peterwilsoncc
2 months ago

Replying to johnbillion:

  • I would rather we didn't introduce logic that runs on every request to handle the Hello Dolly migration.
  • What's the purpose of the wp_should_migrate_hello_dolly filter?
  • Can we get PRs instead of patches too please? So the test suite can run.

Could it be handled in this block of code so it's only caught if the file does not exist?

Alternatively, if plugins that have been deactivated due to a missing file are stored in an option somewhere, the upgrade routine could check for that and enable the relocated plugin that way.

#85 in reply to: ↑ 84 @whyisjake
2 months ago

Replying to peterwilsoncc:

Could it be handled in this block of code so it's only caught if the file does not exist?

That's essentially what's happening here. Check to see if we need to update, and then quickly exit if we don't. Also provides a path to avoid the check altogether.

#86 @jorbin
2 months ago

I don't love the idea of potentially doing a database write on the front end.

Overall though, I am wondering if this overall change is even necessary. While the changes before [60666] and [60670] do help follow the current best practices for plugin development, this is adding a lot of specialized code to accomplish it and I wonder if the benefit is really there.

#87 follow-up: @Otto42
2 months ago

We don't need to change core code for this. It can be handled entirely by the hello.php file that is already there.

Basically, just include the Hello Dolly plugin as an external, much like akismet already is. That way, the new files are there after an update.

Then, replace the contents of the hello PHP file with a small bit of code that does three things. First it checks if the Hello Dolly plugin is there. Second, it rewrites the active plugins option to stop including itself and to include the new hello plugin. Then finally, it simply includes the Hello Dolly plugin directly. This is basically a one-stop shop to the plugin deactivating itself, making itself irrelevant and changing it to including new plugins as part of the normal plugin load system.

Nothing required in core. Nothing needed in updates logic, it's simply a plugin updating its location and then not being loaded anymore. Seems like a very simple process.

#88 @swissspidy
2 months ago

One of the concerns I raised about this change is that the new hello-dolly/hello.php file in core does not match the one in the plugin directory, so any checksum verifications are failing. Seeing this now in WP-CLI tests for instance. Including it as an external would fix this.

#89 @SergeyBiryukov
2 months ago

In 60716:

Coding Standards: Move upgrade_690() to the correct place, after upgrade_682().

Follow-up to [60666].

See #53323.

This ticket was mentioned in PR #9791 on WordPress/wordpress-develop by @peterwilsoncc.


2 months ago
#90

  • Moves the call to upgrade_690() so it actually gets called
  • Wraps Hello Dolly upgrade code in a version check per standard practice
  • Removes the $wpdb->query() call to avoid attempting to create the new index twice, once in the upgrade and once in dbDelta().

Combined fast follows for two tickets:

#91 @SergeyBiryukov
2 months ago

In 60721:

Upgrade/Install: Correct the database upgrade routine for WordPress 6.9.

This commit:

  • Moves the call to upgrade_690() to the correct place so it actually gets called.
  • Wraps Hello Dolly upgrade code in a version check per standard practice.
  • Removes the $wpdb->query() call to avoid attempting to create the new index twice, once in the upgrade and once in dbDelta().

Follow-up to [60666], [60716], [60717].

Props peterwilsoncc, mukesh27.
See #50161, #53323.

@SergeyBiryukov commented on PR #9791:


2 months ago
#92

Thanks for the PR! Merged in r60721.

#93 @dd32
2 months ago

get_option() should have a default of an empty array here, to account for Multisite networks where an individual site doesn't have plugins active.

E_ERROR: Uncaught TypeError: array_search(): Argument #2 ($haystack) must be of type array, string given in wp-admin/includes/upgrade.php:2504

Stack trace:
#0 wp-admin/includes/upgrade.php(2504): array_search('hello.php', '', true)
#1 wp-admin/includes/upgrade.php(890): upgrade_690()
#2 wp-admin/includes/upgrade.php(678): upgrade_all()
#3 wp-admin/upgrade.php(166): wp_upgrade()
#4 {main}
 thrown
Source: GET https://example.org/wp-admin/upgrade.php?step=1

FYI @SergeyBiryukov


edit: Seems that there's no consistency between get_option( 'active_plugins' ), get_option( 'active_plugins', array() ) and (array) get_option( 'active_plugins', array() ): https://github.com/search?q=repo%3AWordPress%2Fwordpress-develop%20get_option(%20%27active_plugins%27&type=code

Last edited 2 months ago by dd32 (previous) (diff)

#94 @SergeyBiryukov
2 months ago

In 60725:

Upgrade/Install: Cast get_option( 'active_plugins' ) to array in upgrade_690().

This resolves an error on Multisite networks where an individual site doesn't have plugins active:

array_search(): Argument #2 ($haystack) must be of type array, string given

Follow-up to [60666], [60721].

Props dd32.
See #53323.

This ticket was mentioned in Slack in #core by welcher. View the logs.


7 weeks ago

#96 @davidbaumwald
7 weeks ago

  • Keywords reporter-feedback removed

Removing reporter-feedback

This ticket was mentioned in Slack in #core by welcher. View the logs.


4 weeks ago

#98 @welcher
4 weeks ago

This was reviewed in the 6.9 bug scrub today. It seems that we need to get aligned to move this across the finish line in time for beta 1 in a couple of weeks.

#99 in reply to: ↑ 87 @afragen
4 weeks ago

Replying to Otto42:

We don't need to change core code for this. It can be handled entirely by the hello.php file that is already there.

Basically, just include the Hello Dolly plugin as an external, much like akismet already is. That way, the new files are there after an update.

Then, replace the contents of the hello PHP file with a small bit of code that does three things. First it checks if the Hello Dolly plugin is there. Second, it rewrites the active plugins option to stop including itself and to include the new hello plugin. Then finally, it simply includes the Hello Dolly plugin directly. This is basically a one-stop shop to the plugin deactivating itself, making itself irrelevant and changing it to including new plugins as part of the normal plugin load system.

Nothing required in core. Nothing needed in updates logic, it's simply a plugin updating its location and then not being loaded anymore. Seems like a very simple process.

@Otto42 any chance you might be able to put the above in code so we can cross the finish line? Please and thank you.

#100 @peterwilsoncc
4 weeks ago

With the deactivation problems, I think the best approach for 6.9 would be to revert the existing changes and if Otto's suggested approach works aim to get that in for the beta release.

This ticket was mentioned in PR #10250 on WordPress/wordpress-develop by @jorbin.


3 weeks ago
#101

Patch prepared in SVN by running the following commands:

svn merge -c -60725 .
svn merge -c -60721 .
svn merge -c -60716 .
svn merge -c -60770 .
svn merge -c -60666 .

Trac ticket: https://core.trac.wordpress.org/ticket/53323

#102 @jorbin
3 weeks ago

  • Keywords needs-patch added; has-patch removed

If someone is able to give @Otto42's idea a shot, I would love to see a patch. In the meantime, I've prepared a PR to confirm the revert.

#103 @afragen
3 weeks ago

FYI, Matt just brought this up in his WCEH AMA.

https://www.youtube.com/watch?v=RL-XccK30sY

#104 @peterwilsoncc
2 weeks ago

In 61006:

Upgrade/Install: Revert relocation of Hello Dolly plugin.

Reverts Hello Dolly changes moving the plugin to a containing folder. Removes the upgrade_690() function in it's entirety as the upgrade routine is no longer required.

Fully reverted commits: [60666], [60670], [60716], [60725]; partially reverts [60721].

Porps johnbillion, whyisjake, SirLouen, mindctrl, afragen, jorbin, Otto42, swissspidy, welcher, davidbaumwald.
See #53323.

#106 @peterwilsoncc
2 weeks ago

  • Keywords has-unit-tests needs-patch removed
  • Milestone changed from 6.9 to 7.0

Moving to the 7.0 milestone for another look with @Otto42's approach suggested in comment #87.

As there were a few conflicts, it wasn't possible to do simple reverts of the earlier commits so it was done manually. I subsequently forgot to do record-only reverse merges sorry.

Note: See TracTickets for help on using tickets.