WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 4 months ago

#9296 reopened defect (bug)

Settings API & Permalink Settings Page Bug

Reported by: jfarthing84 Owned by: jfarthing84
Milestone: Future Release Priority: normal
Severity: major Version: 2.7.1
Component: Permalinks Keywords: has-patch needs-testing 3.6-early settings-api needs-refresh
Focuses: administration Cc:

Description

Although there is a hook in the options-permalink.php to insert custom settings, it does not actually save any custom setting which is added to that page. Instead of posting to options.php like all the other options pages, it posts to itself and only handles the form data which is built into the wordpress core. It should be implemented on that page to also store custom settings that may be hooked onto that page.

Attachments (7)

permalink-settings-fix.diff (21.5 KB) - added by jfarthing84 6 years ago.
Contains options.php, options-head.php & options-permalink.php
9296.diff (5.9 KB) - added by andrewryno 4 years ago.
9296.2.diff (5.6 KB) - added by SergeyBiryukov 2 years ago.
Refreshed
9296.cat-tag-base.diff (862 bytes) - added by nacin 2 years ago.
9296.3.diff (6.1 KB) - added by kovshenin 2 years ago.
Refreshed
9296.4.diff (8.5 KB) - added by kovshenin 2 years ago.
9296.5.diff (898 bytes) - added by nacin 2 years ago.

Download all attachments as: .zip

Change History (59)

comment:1 @jfarthing846 years ago

  • Keywords settings api permalink settings added; removed

comment:2 @jfarthing846 years ago

  • Owner changed from anonymous to jfarthing84
  • Status changed from new to assigned

@jfarthing846 years ago

Contains options.php, options-head.php & options-permalink.php

comment:3 @jfarthing846 years ago

  • Keywords has-patch added; register_setting settings api permalink settings removed
  • Resolution set to fixed
  • Status changed from assigned to closed

In order to make the options-permalink.php page capable of saving custom options hooked in using the add_settings_field() and register_setting() functions, I modified three files. The plan was to make options-permalink.php submit to post.php, like all of the other options pages, instead of submitting to itself and handling it's own post.

I had to modify options-permalink.php by changing the form HTML to post to options.php and also added the settings_fields('permalink') in place of the wp_nonce_field('update_permalink'). I changed the line for the update message handling from $_POSTsubmit? to $_GETupdated? since we will no longer be posting to this page, but the updated value will be passed back upon success. Finally, I removed the whole form processing code to insert into options.php.

With options.php, I added the form processing code from options-permalink.php right before the custom options get saved. I used an if clause to obviously only save those options when options-permalink.php is submitted.

I then found with the modifications made to those two files that upon successfully updating the permalinks, I would receive two different success messages. One was coming from options-permalink.php's own success messages, plus one from options-head.php. So, I modified options-head.php to not display any messages if it is on the options-permalink.php page and allow options-permalink.php to use it's own notifications.

In the end, all is working and you can now save custom settings from the permalinks settings page.

comment:4 @jfarthing846 years ago

  • Milestone changed from Unassigned to 2.7.2

comment:5 @dimadin6 years ago

  • Cc dimadin added
  • Milestone changed from 2.7.2 to 2.8.4
  • Priority changed from normal to high
  • Resolution fixed deleted
  • Status changed from closed to reopened

Since this is still not fixed, people could spend a lot of time trying to figure out where they made mistake in adding field on permalinks page, unaware that there is a bug that prevent them from it.

comment:6 @dd326 years ago

  • Component changed from General to Administration
  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 2.8.4 to 2.9
  • Priority changed from high to normal

This is possibly something which affects other pages.. there may already be an API for this, i'm not sure..

milestone: current trunk for new defects, point release back-ports at commiters discretion, future release for all other defects(long standing)

comment:7 @ryan5 years ago

  • Milestone changed from 2.9 to Future Release

comment:8 @sillybean5 years ago

  • Cc steph@… added
  • Version changed from 2.7.1 to 2.9.1

Definitely affecting other pages. I just tried to add some settings to options-privacy.php and failed. On submitting changes, I get an error on options.php: "Error! Options page not found."

comment:9 @miqrogroove5 years ago

  • Milestone changed from Future Release to 3.0
  • Version changed from 2.9.1 to 2.7.1

reset version

comment:10 @jfarthing845 years ago

  • Cc jeff@… added

comment:11 @dd325 years ago

  • Keywords early added
  • Milestone changed from 3.0 to 3.1

This patch has the right idea in mind.

Does someone feel like updating the patch?

I'm moving this to 3.1 for now due to no patch.

comment:12 @ryan4 years ago

I think I'd rather drop the requirement to post to options.php. Move the validation and update logic into a function that can be called from options-permalink.php, maybe. Make the options pages use the registration API so we don't have to maintain a big array of all default options.

comment:13 @nacin4 years ago

  • Keywords 3.2-early added
  • Milestone changed from Awaiting Triage to Future Release

comment:14 @norbertm4 years ago

  • Cc norbert@… added

comment:15 @johnjamesjacoby4 years ago

  • Cc jjj@… added

Just bumped my head into this for a while trying to merge the bbPress plugin slugs into options-permalink.php. (cc'ed)

comment:16 @andrewryno4 years ago

  • Cc andrewryno@… added

Updated patch for 3.1.

@andrewryno4 years ago

comment:17 @johnjamesjacoby4 years ago

Looking to fast-track this in to 3.2 early for sure. The bbPress plugin and future versions of BuddyPress should rely on hooking into this permalinks screen rather than recreating their own.

comment:18 @andrewryno4 years ago

Looking over my patch, I'm wondering if messages should be handled via add_settings_error() in options.php as well? The permalink messages depend on some variables, so if those messages are moved, those variables will either have to be moved or copied (since they are used elsewhere in options-permalink.php). Ryan said he would like to see the messages moved as well, but just to a different function, not options.php.

Thoughts? I'd be willing to take this on to the best of my abilities, just want some direction on what might be the best route.

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

comment:19 @jkudish4 years ago

  • Keywords has-patch added; needs-patch removed

updating keywords

comment:20 follow-up: @Otto424 years ago

Bump. Can we fix this please? Getting emails about it now.

comment:21 in reply to: ↑ 20 @johnjamesjacoby4 years ago

Replying to Otto42:

Bump. Can we fix this please? Getting emails about it now.

If that's all it takes, allow me to gun it to 88, go back to 2009, and send you a few.

comment:22 @Otto424 years ago

Heh. Possibly this should be merged in with "make all the settings page actually use the settings API" sort of thing?

comment:23 @johnjamesjacoby4 years ago

I imagine this will get picked up as part of the settings screen iterations of 3.3, which is probably part of that, yeh.

comment:24 @SergeyBiryukov4 years ago

  • Keywords 3.2-early removed
  • Milestone changed from Future Release to 3.3

Related: #18285

comment:25 @ryan3 years ago

  • Milestone changed from 3.3 to Future Release

Punting until we finally update the settings API.

comment:26 @johnjamesjacoby3 years ago

What can I do to make this happen in 3.4?

comment:27 @scribu3 years ago

  • Cc scribu added

comment:28 @bpetty2 years ago

  • Cc bpetty added
  • Keywords needs-patch needs-refresh added; has-patch removed

Related: #21488

Latest patch from @andrewryno is stale now too, but there seems to be more interest in #18285 and #21488, which both affect anything written to fix this (and should fix this ticket in the process).

@SergeyBiryukov2 years ago

Refreshed

comment:29 @wonderboymusic2 years ago

  • Keywords has-patch added; early needs-patch needs-refresh removed
  • Milestone changed from Future Release to 3.5

Related #21824, #18285

Last edited 2 years ago by wonderboymusic (previous) (diff)

comment:30 @scribu2 years ago

Until we revamp the whole settings handling code, I guess it's better to have consistency, as an intermediate step.

Could we move that whole block inside if ( 'permalink' == $option_page ) { to a function, though? There should be a hook for it, right?

Last edited 2 years ago by scribu (previous) (diff)

comment:31 @nacin2 years ago

Okay, I'm convinced. Seems like this could help #14345 and others.

@nacin2 years ago

comment:32 @nacin2 years ago

Let's see if we can make this work more like everything else, even if just a little.

  • 9296.cat-tag-base.diff shows how category_base and tag_base can get pulled into a sanitize_option() callback. Doing this for permalink_structure is a bit more tricky.
  • But if we are able to handle permalink_structure through sanitize_option(), we could issue our own add_settings_error() call to handle our custom 'Permalink structure updated.' message.
  • A lot of variables used in the options.php branch aren't set.

In general, I'd like it if options.php can stay as generalized as possible. Let's see what we can do to make this work.

comment:33 @greenshady2 years ago

  • Cc justin@… added

comment:34 @mordauk2 years ago

  • Cc pippin@… added

comment:35 @emzo2 years ago

  • Cc wordpress@… added

@kovshenin2 years ago

Refreshed

comment:36 @kovshenin2 years ago

In 9296.3.diff tested and refreshed. Seems to work well for me and for anything registered to permalink with register_setting. We can also move settings errors/updates out from options-permalink.php and into options.php to keep things closer to each other, and to avoid the 'permalink' != $option_page hack around add_settings_error in options.php. What do you think?

Version 0, edited 2 years ago by kovshenin (next)

comment:37 @SergeyBiryukov2 years ago

Nitpicking: not sure if it actually matters in terms of validation in HTML5, but I guess in 9296.3.diff, method="post" in line 135 of options-permalink.php should stay in lower case. See [14626] for a related change.

@kovshenin2 years ago

comment:38 @kovshenin2 years ago

  • Keywords needs-testing added

Thanks @SergeyBiryukov!

In 9296.4.diff fixed the updating of the permalink structure in .3 and .2. Moved settings errors to options.php with add_settings_error, though I now feel bad about code duplication in options.php and options-permalink.php, however, hopefully it can all be easily cleaned up and migrated when #18285 is in place.

I also tested against #14345 and could not reproduce the issue with the new patch, though I'd love someone to test it out, especially on IIS.

Thanks!

@nacin2 years ago

comment:39 @nacin2 years ago

  • Milestone changed from 3.5 to Future Release

comment:40 @johnjamesjacoby2 years ago

  • Keywords 3.6-early added

comment:41 @unknowndomain2 years ago

  • Keywords settings-3.6 added

comment:42 @unknowndomain2 years ago

  • Cc me@… added

comment:43 @chipbennett2 years ago

  • Cc chip@… added

comment:44 @knutsp2 years ago

  • Cc knut@… added

comment:45 @ctsttom2 years ago

  • Keywords needs-refresh added

@nacin / @kovshenin / @johnnamesjacoby / @jfarthing84

Would you be interested in refreshing this code once 3.5.1 is out in the next couple of weeks to get this into 3.6?

comment:46 @SergeyBiryukov2 years ago

  • Keywords settings-api added; settings-3.6 removed

comment:47 @pauldewouters2 years ago

  • Cc pauldewouters@… added

comment:48 @nacin23 months ago

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

In 24060:

Redirect after save on options-permalink.php to ensure permalinks are fully flushed. fixes #9296. see #14345 for more.

comment:49 @nacin23 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I switched the two ticket references in [24060] — fixes #14345, see #9296 for more.

comment:50 @nacin13 months ago

  • Component changed from Administration to Permalinks
  • Focuses administration added

comment:51 follow-up: @captaintheme7 months ago

Any plans for this one soon?

comment:52 in reply to: ↑ 51 @cyman4 months ago

Replying to captaintheme:

Any plans for this one soon?

+1

Note: See TracTickets for help on using tickets.