Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#35936 closed enhancement (fixed)

Add validation for Custom Structure permalinks

Reported by: stevenetrivet's profile SteveNetRivet Owned by: swissspidy's profile swissspidy
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Permalinks Keywords: good-first-bug has-patch has-unit-tests commit
Focuses: ui Cc:

Description

As a premium theme developer, we see many users confused by the Custom Structure option in the Permalinks settings screen. They often type in static paths without any structure tags - for example, typing in "About" which breaks all internal linking except their page which happens to have this slug.

Please add to this settings box some level of validation so that users cannot save changes if their permalink structure contains no structure tags in the Custom Structure box. Consider including helpful feedback and a link to the Help > Permalink Settings dropdown found in this settings area for further reading.

Attachments (9)

35936.patch (2.0 KB) - added by rockwell15 8 years ago.
35936.2.patch (1.6 KB) - added by rockwell15 8 years ago.
35936.3.patch (1.7 KB) - added by rockwell15 8 years ago.
Updated the error message
35936.diff (1.7 KB) - added by swissspidy 8 years ago.
35936.5.patch (1.7 KB) - added by rockwell15 8 years ago.
Updated the error message & link
35936.2.diff (3.4 KB) - added by swissspidy 8 years ago.
35936.3.diff (3.4 KB) - added by swissspidy 8 years ago.
Simplify the error string
35936.4.diff (5.6 KB) - added by swissspidy 8 years ago.
35936.5.diff (5.0 KB) - added by swissspidy 8 years ago.

Download all attachments as: .zip

Change History (31)

#1 @swissspidy
8 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release
  • Version 4.4.2 deleted

Please add to this settings box some level of validation so that users cannot save changes if their permalink structure contains no structure tags in the Custom Structure box.

Such a validation is pretty easily doable. We could check for existence of the % sign alone or even the available tags themselves (%postname% et al).

At least the documentation could be improved, perhaps warning users that they could easily render their site unusable with a bad setting.

See also: #29872

@rockwell15
8 years ago

#2 @rockwell15
8 years ago

  • Keywords has-patch added; needs-patch removed

#3 @johnbillion
8 years ago

  • Keywords needs-patch added; has-patch removed

@rockwell15 Thanks for the patch, but enforcing %post_id% or %postname% as the last element in the permalink structure is too restrictive. I've seen sites with funky structures that include things such as a trailing /read element.

I think testing for the existence of a % symbol is enough validation.

@rockwell15
8 years ago

@rockwell15
8 years ago

Updated the error message

#4 @rockwell15
8 years ago

  • Keywords has-patch added; needs-patch removed

@johnbillion Ok, thanks. I updated the patch

@swissspidy
8 years ago

#5 @swissspidy
8 years ago

@rockwell15 The latest patch works well. Some notes:

Coding standards in general should be followed, see https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/. For example, strict comparison should be used in conditions (e.g. false === strpos... instead of false == strpos...). Always use braces, etc. Also, the error message needs to be translatable.

While testing the patch I realized that sanitize_option() already does the error handling for us. 35936.diff is an alternative approach taking advantage of that. Wording of the error message needs to be improved though.

#6 @swissspidy
8 years ago

  • Milestone changed from Future Release to 4.6

#7 @rockwell15
8 years ago

@swissspidy Thanks for the tips!

So the only thing left to do for this ticket is improve the wording of the error message?

#8 @swissspidy
8 years ago

@rockwell15 Yeah, I suggest going forward with 35936.diff and fine-tune the error message.

We need something between "No structure tag used! Learn more" and "Make sure to include a structure tag." (from 35936.3.patch). Not too harsh, not too long, and informative about what went wrong.

#9 follow-up: @rockwell15
8 years ago

@swissspidy Hows about "A structure tag is required when using custom permalinks."

Also, what do you think about changing the link to jump to the custom permalinks section so it's easier to follow:
https://codex.wordpress.org/Using_Permalinks#Choosing_your_permalink_structure

#10 in reply to: ↑ 9 @swissspidy
8 years ago

  • Owner set to rockwell15
  • Status changed from new to assigned

Replying to rockwell15:

@swissspidy Hows about "A structure tag is required when using custom permalinks."

Also, what do you think about changing the link to jump to the custom permalinks section so it's easier to follow:
https://codex.wordpress.org/Using_Permalinks#Choosing_your_permalink_structure

Yeah that could work.

@rockwell15
8 years ago

Updated the error message & link

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


8 years ago

#12 @chriscct7
8 years ago

  • Owner changed from rockwell15 to swissspidy
  • Status changed from assigned to reviewing

#13 @chriscct7
8 years ago

  • Keywords needs-unit-tests added

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


8 years ago

#15 @voldemortensen
8 years ago

  • Milestone changed from 4.6 to Future Release

Punting from 4.6 unless a patch with unit tests and a review comes soon.

@swissspidy
8 years ago

#16 @swissspidy
8 years ago

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

35936.2.diff makes the permalink structure more strict and adds a test for the sanitization part.

@swissspidy
8 years ago

Simplify the error string

#17 @ocean90
8 years ago

35936.3.diff looks good so far.
For the placeholder we should add a translator comment /* translators: %s: Codex URL */. And there should be a redirect in case of an error too (PRG pattern). options-permalink.php?error=true should be fine.

@swissspidy
8 years ago

#18 @swissspidy
8 years ago

What seems like an easy task has once again proven to be quite a mess…

35936.4.diff adds the translator comment and keeps the redirect in any case. Since sanitize_option() relies on add_settings_error() I had to rewrite the whole message handling to use add_settings_error() / get_settings_errors(). That's why the diff looks so huge, when it basically only brings options-permalink.php in line with the rest of the admin using the settings API.

#19 @ocean90
8 years ago

35936.4.diff: The change to src/wp-admin/admin-header.php isn't necessary, $parent_file is options-general.php for all options-* screens.

The block

if ( ! get_settings_errors() ) { 
	$wp_rewrite->set_permalink_structure( $permalink_structure ); 
}

can be removed too. $permalink_structure will be the same as before and WP_Rewrite::set_permalink_structure() does a $permalink_structure != $this->permalink_structure check. get_settings_errors() is also unreliable because it returns errors for all settings.

@swissspidy
8 years ago

#20 @swissspidy
8 years ago

  • Milestone changed from Future Release to 4.6

Thanks for your valuable feedback! 35936.5.diff is the fifth round of improvement.

Moving to 4.6 for consideration.

#21 @ocean90
8 years ago

  • Keywords commit added

#22 @swissspidy
8 years ago

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

In 37747:

Permalinks: Validate custom permalink structures.

Custom permalink structures require at least one valid structure tag, e.g. %postname%. If none is included, it would leave users with broken permalinks.
Let's make sure this won't happen by validating the permalink structure.

Adds unit tests.

Props rockwell15 for initial patch.
Fixes #35936.

Note: See TracTickets for help on using tickets.