WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#38928 closed defect (bug) (fixed)

Default theme content - problem with slugs

Reported by: pavelevap Owned by: westonruter
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch needs-testing has-unit-tests commit dev-reviewed
Focuses: Cc:

Description

I installed latest RC1-39355, activated Twenty Seventeen and went to Customizer.

There were 5 pages created, but with number in slug, for example contact-2, blog-2, etc. It is a fresh install and I did not have any pages created before. For every created page I have 3 records in database:

- page: auto-draft > a-homepage-section
- page: publish > a-homepage-section-2
- revision: inherit > 20-revision-v1

I do not think it is necessary? When default page during WP installation is created, there is only one publish record in database.

Attachments (7)

38928.0.diff (664 bytes) - added by westonruter 5 months ago.
38928.1.diff (1.4 KB) - added by westonruter 5 months ago.
38928.2.diff (2.0 KB) - added by westonruter 5 months ago.
38928.3.diff (9.5 KB) - added by westonruter 5 months ago.
38928.4.diff (12.0 KB) - added by westonruter 5 months ago.
Adding unit tests for wp_unique_post_slug() changes: https://github.com/xwp/wordpress-develop/pull/210
38928.5.diff (22.0 KB) - added by westonruter 5 months ago.
Δ https://github.com/xwp/wordpress-develop/pull/210/commits/eaa15694325e82a2af00d40717a49f6fff2ce6d6
38928.6.diff (22.4 KB) - added by westonruter 5 months ago.
Δ https://github.com/xwp/wordpress-develop/pull/210/commits/0372007634753eaf12710f3c4e3dcbecfd5ab428

Download all attachments as: .zip

Change History (23)

This ticket was mentioned in Slack in #core-customize by helen. View the logs.


5 months ago

#2 @lukecavanagh
5 months ago

@pavelevap

Created the starter content just fine, from the customizer on local dev site which was a fresh install. No duplicate slug issue found.

#3 @westonruter
5 months ago

  • Component changed from General to Customize
  • Milestone changed from Awaiting Review to 4.7

I can replicate this issue. It happens if you open the customizer, close it, and then open it again so that the starter content gets imported a second time. When you do that, you currently get a posts table that looks like:

+----+--------------------+--------------------+---------------------+-------------+
| ID | post_title         | post_name          | post_date           | post_status |
+----+--------------------+--------------------+---------------------+-------------+
| 19 | A homepage section | a-homepage-section | 2016-11-28 23:08:28 | auto-draft  |
| 18 | Blog               | blog               | 2016-11-28 23:08:28 | auto-draft  |
| 17 | Contact            | contact            | 2016-11-28 23:08:28 | auto-draft  |
| 16 | About              | about              | 2016-11-28 23:08:28 | auto-draft  |
| 15 | Home               | home               | 2016-11-28 23:08:28 | auto-draft  |
| 10 | A homepage section | a-homepage-section | 2016-11-28 23:08:16 | auto-draft  |
| 9  | Blog               | blog               | 2016-11-28 23:08:16 | auto-draft  |
| 8  | Contact            | contact            | 2016-11-28 23:08:16 | auto-draft  |
| 7  | About              | about              | 2016-11-28 23:08:16 | auto-draft  |
| 6  | Home               | home               | 2016-11-28 23:08:16 | auto-draft  |
+----+--------------------+--------------------+---------------------+-------------+

There are two fixes we can consider here, which aren't mutually exclusive:

For one, the logic for wp_unique_post_slug() can be changed to explicitly ignore auto-draft posts when it is doing the lookup to see if a slug is already being used. This seems like a no-brainer: 38928.2.diff

The second fix would be specifically to account for when we allow starter content to be applied on existing sites. Namely, when we do our lookup to see if there are existing auto-draft posts in the current customized state that have the given slugs… we could expand this to see if there are any non-auto-draft posts _outside_ the customized state which already have those slugs, and then re-use them instead of inserting new auto-drafts.

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

@westonruter
5 months ago

#4 @westonruter
5 months ago

  • Keywords has-patch added
  • Owner set to westonruter
  • Status changed from new to accepted

#5 @westonruter
5 months ago

And actually I think wp_unique_post_slug() should include the post_type in the query since it is fine for posts of different types to have the same slug, right?

@westonruter
5 months ago

#6 @westonruter
5 months ago

Nevermind, I see for non-attachment posts, it is already explicitly checking the post_type as well. See 38928.2.diff for update.

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

@westonruter
5 months ago

@westonruter
5 months ago

#7 @westonruter
5 months ago

  • Keywords needs-unit-tests added

@helen 38928.3.diff includes the logic for re-using non-auto-draft posts instead of adding new auto-drafts for the starter content.

#8 @pavelevap
5 months ago

Great, I do not remember that I returned back to Customizer. I installed RC version without Twenty Seventeen (so Twenty Sixteen was activated by default), then added Twenty Seventeen and activated it manually. Then looked at the site and then went to Customizer to set up homepage. But I do not think that I returned to Customizer (I will test it once again).

And also attachments were duplicated and pictures imported twice (with all related sizes).

#9 @westonruter
5 months ago

  • Keywords needs-testing added

@pavelevap Can you confirm the patch here improves the expected experience?

@westonruter
5 months ago

Adding unit tests for wp_unique_post_slug() changes: https://github.com/xwp/wordpress-develop/pull/210

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


5 months ago

#11 @westonruter
5 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

In writing more unit tests, as always happens, I found some gaps in starter content. Namely, it was not possible to set starter content for custom logo, background, or header. The latest patch ensures these are supported via unit tests and also it adds tests for this ticket's original issue, which was re-using non-auto-draft posts and attachments that have matching slugs.

#12 @helen
5 months ago

  • Keywords commit added

Patch looks good to me, also fixes a pretty odd bug I happened to come across just before I tested this patch, where already-published pages were getting their slugs appended when I went to edit them.

The one concern I have (that doesn't block commit, but needs discussion) is what then is the difference between default header and background images vs. ones provided in starter content. There's the different in sideloading, but is there one that should be preferred / recommended over the other?

#13 @westonruter
5 months ago

  • Keywords dev-reviewed added

#14 @westonruter
5 months ago

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

In 39411:

Customize: Reuse existing non-auto-draft posts and existing auto-draft posts in the customized state with matching slugs when applying starter content.

  • Updates wp_unique_post_slug() to ignore auto-draft posts. Prevents publishing multiple posts that have the same slugs from starter content.
  • Fixes fatal error when attempting to save an header_image setting from a non-admin context.
  • Fixes substituting attachment symbols in options and theme mods.
  • Fixes applying starter content for header images and background images.

See #38114.
Fixes #38928.

#15 @westonruter
5 months ago

In 39412:

Customize: Reuse existing non-auto-draft posts and existing auto-draft posts in the customized state with matching slugs when applying starter content.

  • Updates wp_unique_post_slug() to ignore auto-draft posts. Prevents publishing multiple posts that have the same slugs from starter content.
  • Fixes fatal error when attempting to save an header_image setting from a non-admin context.
  • Fixes substituting attachment symbols in options and theme mods.
  • Fixes applying starter content for header images and background images.

Merges [39411] to 4.7 branch.
See #38114.
Fixes #38928 for 4.7.

#16 @westonruter
5 months ago

In 39506:

Customize: Defer populating post_name for auto-draft posts in customized state until posts are published.

The ultimate post_name is stored in postmeta until the post is published. The get_page_by_path() function does not exclude auto-draft posts. Revert changes to wp_unique_post_slug() from [39411] which excluded auto-draft posts.

Props westonruter, dlh for testing, helen for testing.
See #38114, #38928.
Fixes #39078.

Note: See TracTickets for help on using tickets.