WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 5 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 5 years ago.
Contains options.php, options-head.php & options-permalink.php
9296.diff (5.9 KB) - added by andrewryno 3 years ago.
9296.2.diff (5.6 KB) - added by SergeyBiryukov 22 months ago.
Refreshed
9296.cat-tag-base.diff (862 bytes) - added by nacin 22 months ago.
9296.3.diff (6.1 KB) - added by kovshenin 21 months ago.
Refreshed
9296.4.diff (8.5 KB) - added by kovshenin 21 months ago.
9296.5.diff (898 bytes) - added by nacin 20 months ago.

Download all attachments as: .zip

Change History (57)

comment:1 jfarthing845 years ago

  • Keywords settings api permalink settings added; removed

comment:2 jfarthing845 years ago

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

jfarthing845 years ago

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

comment:3 jfarthing845 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 jfarthing845 years ago

  • Milestone changed from Unassigned to 2.7.2

comment:5 dimadin5 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 dd325 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 sillybean4 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 miqrogroove4 years ago

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

reset version

comment:10 jfarthing844 years ago

  • Cc jeff@… added

comment:11 dd324 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 johnjamesjacoby3 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 andrewryno3 years ago

  • Cc andrewryno@… added

Updated patch for 3.1.

andrewryno3 years ago

comment:17 johnjamesjacoby3 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 andrewryno3 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 3 years ago by andrewryno (previous) (diff)

comment:19 jkudish3 years ago

  • Keywords has-patch added; needs-patch removed

updating keywords

comment:20 follow-up: Otto423 years ago

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

comment:21 in reply to: ↑ 20 johnjamesjacoby3 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 Otto423 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 johnjamesjacoby3 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 SergeyBiryukov3 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 johnjamesjacoby2 years ago

What can I do to make this happen in 3.4?

comment:27 scribu2 years ago

  • Cc scribu added

comment:28 bpetty23 months 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).

SergeyBiryukov22 months ago

Refreshed

comment:29 wonderboymusic22 months ago

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

Related #21824, #18285

Last edited 22 months ago by wonderboymusic (previous) (diff)

comment:30 scribu22 months 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 22 months ago by scribu (previous) (diff)

comment:31 nacin22 months ago

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

nacin22 months ago

comment:32 nacin22 months 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 greenshady22 months ago

  • Cc justin@… added

comment:34 mordauk22 months ago

  • Cc pippin@… added

comment:35 emzo22 months ago

  • Cc wordpress@… added

kovshenin21 months ago

Refreshed

comment:36 kovshenin21 months 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?

Update: actually ignore this, 9263.3.diff doesn't work. Working on it :)

Last edited 21 months ago by kovshenin (previous) (diff)

comment:37 SergeyBiryukov21 months 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.

kovshenin21 months ago

comment:38 kovshenin21 months 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!

nacin20 months ago

comment:39 nacin20 months ago

  • Milestone changed from 3.5 to Future Release

comment:40 johnjamesjacoby20 months ago

  • Keywords 3.6-early added

comment:41 unknowndomain19 months ago

  • Keywords settings-3.6 added

comment:42 unknowndomain19 months ago

  • Cc me@… added

comment:43 chipbennett19 months ago

  • Cc chip@… added

comment:44 knutsp19 months ago

  • Cc knut@… added

comment:45 ctsttom19 months 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 SergeyBiryukov19 months ago

  • Keywords settings-api added; settings-3.6 removed

comment:47 pauldewouters17 months ago

  • Cc pauldewouters@… added

comment:48 nacin15 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 nacin15 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 nacin5 months ago

  • Component changed from Administration to Permalinks
  • Focuses administration added
Note: See TracTickets for help on using tickets.