Make WordPress Core

Opened 15 years ago

Last modified 3 years ago

#9296 reopened defect (bug)

Settings API & Permalink Settings Page Bug

Reported by: jfarthing84's profile jfarthing84 Owned by: jfarthing84's profile jfarthing84
Milestone: Future Release Priority: normal
Severity: major Version: 2.7.1
Component: Permalinks Keywords: needs-patch settings-api
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 15 years ago.
Contains options.php, options-head.php & options-permalink.php
9296.diff (5.9 KB) - added by andrewryno 13 years ago.
9296.2.diff (5.6 KB) - added by SergeyBiryukov 11 years ago.
Refreshed
9296.cat-tag-base.diff (862 bytes) - added by nacin 11 years ago.
9296.3.diff (6.1 KB) - added by kovshenin 11 years ago.
Refreshed
9296.4.diff (8.5 KB) - added by kovshenin 11 years ago.
9296.5.diff (898 bytes) - added by nacin 11 years ago.

Download all attachments as: .zip

Change History (66)

#1 @jfarthing84
15 years ago

  • Keywords settings api permalink settings added; removed

#2 @jfarthing84
15 years ago

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

@jfarthing84
15 years ago

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

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

#4 @jfarthing84
15 years ago

  • Milestone changed from Unassigned to 2.7.2

#5 @dimadin
15 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.

#6 @dd32
15 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)

#7 @ryan
14 years ago

  • Milestone changed from 2.9 to Future Release

#8 @sillybean
14 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."

#9 @miqrogroove
14 years ago

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

reset version

#10 @jfarthing84
14 years ago

  • Cc jeff@… added

#11 @dd32
14 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.

#12 @ryan
14 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.

#13 @nacin
13 years ago

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

#14 @norbertm
13 years ago

  • Cc norbert@… added

#15 @johnjamesjacoby
13 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)

#16 @andrewryno
13 years ago

  • Cc andrewryno@… added

Updated patch for 3.1.

@andrewryno
13 years ago

#17 @johnjamesjacoby
13 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.

#18 @andrewryno
13 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 13 years ago by andrewryno (previous) (diff)

#19 @jkudish
13 years ago

  • Keywords has-patch added; needs-patch removed

updating keywords

#20 follow-up: @Otto42
13 years ago

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

#21 in reply to: ↑ 20 @johnjamesjacoby
13 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.

#22 @Otto42
13 years ago

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

#23 @johnjamesjacoby
13 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.

#24 @SergeyBiryukov
13 years ago

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

Related: #18285

#25 @ryan
12 years ago

  • Milestone changed from 3.3 to Future Release

Punting until we finally update the settings API.

#26 @johnjamesjacoby
12 years ago

What can I do to make this happen in 3.4?

#27 @scribu
12 years ago

  • Cc scribu added

#28 @bpetty
12 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).

@SergeyBiryukov
11 years ago

Refreshed

#29 @wonderboymusic
11 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 11 years ago by wonderboymusic (previous) (diff)

#30 @scribu
11 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 11 years ago by scribu (previous) (diff)

#31 @nacin
11 years ago

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

#32 @nacin
11 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.

#33 @greenshady
11 years ago

  • Cc justin@… added

#34 @mordauk
11 years ago

  • Cc pippin@… added

#35 @emzo
11 years ago

  • Cc wordpress@… added

@kovshenin
11 years ago

Refreshed

#36 @kovshenin
11 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?

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

Last edited 11 years ago by kovshenin (previous) (diff)

#37 @SergeyBiryukov
11 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.

@kovshenin
11 years ago

#38 @kovshenin
11 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!

@nacin
11 years ago

#39 @nacin
11 years ago

  • Milestone changed from 3.5 to Future Release

#40 @johnjamesjacoby
11 years ago

  • Keywords 3.6-early added

#41 @unknowndomain
11 years ago

  • Keywords settings-3.6 added

#42 @unknowndomain
11 years ago

  • Cc me@… added

#43 @chipbennett
11 years ago

  • Cc chip@… added

#44 @knutsp
11 years ago

  • Cc knut@… added

#45 @ctsttom
11 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?

#46 @SergeyBiryukov
11 years ago

  • Keywords settings-api added; settings-3.6 removed

#47 @pauldewouters
11 years ago

  • Cc pauldewouters@… added

#48 @nacin
11 years 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.

#49 @nacin
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#50 @nacin
10 years ago

  • Component changed from Administration to Permalinks
  • Focuses administration added

#51 follow-up: @captaintheme
10 years ago

Any plans for this one soon?

#52 in reply to: ↑ 51 @cyman
9 years ago

Replying to captaintheme:

Any plans for this one soon?

+1

#53 @helgatheviking
9 years ago

Is this going anywhere or should we assume that after 7 years it is a "wontfix"?

#54 @DrewAPicture
8 years ago

  • Keywords needs-patch added; has-patch needs-testing 3.6-early settings-api needs-refresh removed

@chriscct7: Any interest in taking a stab at this as part of the other settings changes?

#55 @afercia
7 years ago

  • Keywords settings-api added

#56 @DrLightman
5 years ago

Still broken: proper settings API setup won't save the user settings.
Must hook into:
add_action('load-options-permalink.php', 'my_load_options_permalink_callback')); ('admin_post' wont work)
and process and save into database $_POST data manually.

Last edited 5 years ago by DrLightman (previous) (diff)

#57 @styledev
5 years ago

I can't believe this is still not fixed after a decade!

+1 @DrLightman for the workaround.

#58 @afercia
5 years ago

Related: #38734

WordPress has had a Settings API for eight years, but the core settings screens in WordPress don't use it.

See also https://core.trac.wordpress.org/query?status=!closed&keywords=~settings-api

#59 @RavanH
3 years ago

I seriously cannot believe WordPress is pushing forward so hard while stuff like this is still so badly lagging behind under the hood... Painful and, quite frankly, a little worrying.

Note: See TracTickets for help on using tickets.