Opened 4 years ago
Last modified 3 months ago
#53323 assigned enhancement
Place Hello Dolly in containing folder
Reported by: | afragen | Owned by: | afragen |
---|---|---|---|
Milestone: | 6.8 | Priority: | normal |
Severity: | normal | Version: | 5.8 |
Component: | Upgrade/Install | Keywords: | has-patch needs-testing has-unit-tests |
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
Change History (45)
This ticket was mentioned in PR #1330 on WordPress/wordpress-develop by afragen.
4 years ago
#1
This ticket was mentioned in Slack in #core by afragen. View the logs.
4 years ago
#3
@
4 years ago
- Keywords needs-unit-tests added
Looks like it needs an update to some of the unit tests.
#5
@
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.
#7
@
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
#10
follow-up:
↓ 18
@
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
@
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.
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:
↓ 25
@
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
@
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
@
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.
3 years ago
#22
@
3 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.
3 years ago
This ticket was mentioned in Slack in #core-committers by afragen. View the logs.
22 months ago
#25
in reply to:
↑ 17
@
22 months 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.
#26
@
22 months 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.
22 months ago
This ticket was mentioned in Slack in #core-test by afragen. View the logs.
22 months ago
This ticket was mentioned in Slack in #core-test by afragen. View the logs.
6 months ago
This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.
6 months ago
This ticket was mentioned in Slack in #core by afragen. View the logs.
5 months ago
@peterwilsoncc commented on PR #1330:
4 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.
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.
4 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.
4 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:
4 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
@
4 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
4 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:
4 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.
@peterwilsoncc commented on PR #1330:
4 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
@
4 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
@
3 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.8
Future 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.
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