Make WordPress Core

Opened 3 years ago

Last modified 15 months ago

#53323 assigned enhancement

Place Hello Dolly in containing folder

Reported by: afragen's profile afragen Owned by: afragen's profile afragen
Milestone: Future Release 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 (28)

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


3 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.


3 years ago

#3 @afragen
3 years ago

  • Keywords needs-unit-tests added

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

#4 @afragen
3 years ago

  • Keywords needs-unit-tests removed

Tests updated

#5 @SergeyBiryukov
3 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 3 years ago by SergeyBiryukov (previous) (diff)

#6 @afragen
3 years ago

I can do that tomorrow.

#7 @afragen
3 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.


3 years ago

#9 @afragen
3 years ago

  • Milestone changed from Awaiting Review to 5.9

#10 follow-up: @afragen
3 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.


3 years ago

#12 @afragen
3 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 3 years ago by afragen (previous) (diff)

#13 @afragen
3 years ago

  • Milestone changed from 5.9 to 5.8

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


3 years ago

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


3 years ago

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


3 years ago

#17 follow-up: @justinahinon
3 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
3 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.


3 years ago

#20 @desrosj
3 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 @hellofromTonya
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.


2 years ago

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


15 months ago

#25 in reply to: ↑ 17 @afragen
15 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. No fatal occurs.

Cc @hellofromtonya @boniu91 @pbearne.

Version 3, edited 15 months ago by afragen (previous) (next) (diff)

#26 @afragen
15 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.


15 months ago

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


15 months ago

Note: See TracTickets for help on using tickets.