Make WordPress Core

Opened 14 years ago

Closed 11 years ago

#16884 closed defect (bug) (fixed)

Wrong order of parameters in str_replace() in setup-config's get_bloginfo()

Reported by: hakre's profile hakre Owned by: dd32's profile dd32
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.1
Component: Upgrade/Install Keywords: has-patch dev-feedback
Focuses: Cc:

Description

See str_replace()

Replacing some string with something in an empty string is obviously bogus.

Attachments (5)

16884.patch (598 bytes) - added by hakre 14 years ago.
removal of bogus code
16884.2.patch (723 bytes) - added by hakre 14 years ago.
Parameter Order Fix
16884.3.patch (604 bytes) - added by hakre 14 years ago.
As needed/noramlly passed to the API
16884 (600 bytes) - added by hakre 14 years ago.
wp_guess_url()
16884.4.patch (600 bytes) - added by hakre 14 years ago.
wp_guess_url()

Download all attachments as: .zip

Change History (25)

@hakre
14 years ago

removal of bogus code

#1 @westi
14 years ago

It looks more like someone typed the arguments in the wrong order when trying to make setup-config.php install in a subdir safe.

So we should test that and fix that if it is the real bug.

@hakre
14 years ago

Parameter Order Fix

#2 @hakre
14 years ago

Fix if the first parameter was mixed with the third one and the second one with the first one and the third one with the first one.

#3 follow-up: @hakre
14 years ago

Related: [13232]; #12159; 12159.4.diff ;

As the change was introduced 13 month ago and no error has been reported about it, I would furthermost assume that the replacement is not of a vital need. More than a year of testing incl. with live sites is a pretty long time IMHO so from usage data a replacement is not needed.

Anyway the change has been performed by nacin and he probably remembers why this went in so he can actually clarify for what that str_replace() call was needed.

#4 @hakre
14 years ago

@nacin: For what does that /**#@+ stand for?

#5 @hakre
14 years ago

  • Keywords dev-feedback added

#6 follow-up: @dd32
14 years ago

that change was to allow the HTTP API to be included, which by default, calls get_bloginfo() for the default user agent.

The result of this is that the API request has always set it's user agent to http://host/ instead of http://host/some/directory/. For all intents this hasn't caused an issue as the API doesn't require the URL for the salt generation.

For what does that /#@+ stand for?

It's a special PHPDoc syntax which basically means, Ignore the following functions in the PHPDoc / XRef

#7 in reply to: ↑ 6 @hakre
14 years ago

Replying to dd32:

that change was to allow the HTTP API to be included, which by default, calls get_bloginfo() for the default user agent.

The result of this is that the API request has always set it's user agent to http://host/ instead of http://host/some/directory/. For all intents this hasn't caused an issue as the API doesn't require the URL for the salt generation.

Thanks for the information. Then the replacement was not needed at all I assume.

For what does that /#@+ stand for?

It's a special PHPDoc syntax which basically means, Ignore the following functions in the PHPDoc / XRef

I first thought it would be PHPDoc syntax but could not find any information about it. @ignore is enough for the next element (the function) IMHO.

But I was not thinking about PHPXRef for which it then is needed only? Or is this really a PHPDoc feature?

@hakre
14 years ago

As needed/noramlly passed to the API

#8 @hakre
14 years ago

I incorporated the feedback by dd32 into this patch.

#9 @dd32
14 years ago

The purpose of the str_replace() would've been to guess the real installation URL, and provide a consistent return value between the stub and the real function.

While it makes no practical difference what the function returns, It might as well return the correct guessed url, therefor, 16884.2.patch is probably the correct option here.

#10 @hakre
14 years ago

If blind, second patch is safest bet.

I'm just learning this:

  1. In case no /wp-config.php file exists (e.g. before installation or on error), the user is motivated to request /wp-admin/setup-config.php.
  2. This means that /wp-admin/setup-config.php is called on installing the software.
  3. There is no other place in core that links nor includes /wp-admin/setup-config.php.

So by usage, $_SERVER['PHP_SELF'] is most probably /wp-admin/setup-config.php, at least if it contains the default value (PHP Manual).

#11 @hakre
14 years ago

  1. The function is defined to be called as get_bloginfo( 'url' ) later on in WP_Http::request().
  2. get_bloginfo( 'url' ) is the same as home_url()
  3. home_url() is get_home_url(null, '')
  4. get_home_url(null, '') is basically get_option( 'home' ).

That option does not contain the trailing slash.

#12 @hakre
14 years ago

Third patch is wrong compared with default get_bloginfo( 'url' ) behavior, but should not pose any problems. Same for the first patch.

#13 follow-up: @nacin
14 years ago

It's a parameter order issue. I did subject, match, replace, instead of match, replace, subject.

The phpdoc allows multiple declarations to all take the same phpdoc. I'm not sure it's necessary here as it is one function.

@hakre
14 years ago

wp_guess_url()

@hakre
14 years ago

wp_guess_url()

#14 in reply to: ↑ 13 @hakre
14 years ago

Replying to nacin:

It's a parameter order issue. I did subject, match, replace, instead of match, replace, subject.

Thanks for clarification.

The phpdoc allows multiple declarations to all take the same phpdoc. I'm not sure it's necessary here as it is one function.

Ah, cool.


As the parameter issue could be clarified, this clarified the use of the str_replace statement as well.

I created another patch that is making use of the wp_guess_url() function as it is used to populate get_option( 'home' ) on install. It's available through /wp-includes/functions.php (thx t31os).

#15 @hakre
14 years ago

Related: #8593

#16 @hakre
14 years ago

  • Summary changed from str_replace() in setup-config's get_bloginfo() is bogus to URL generation in setup-config's get_bloginfo()

#17 in reply to: ↑ 3 @SergeyBiryukov
12 years ago

  • Component changed from General to Upgrade/Install
  • Milestone changed from Awaiting Review to Future Release

Replying to hakre:

Related: [13232]; #12159; 12159.4.diff ;

Actually, introduced in [13026].

Closed #20760 as a duplicate.

#18 @SergeyBiryukov
11 years ago

  • Summary changed from URL generation in setup-config's get_bloginfo() to Wrong order of parameters in str_replace() in setup-config's get_bloginfo()

#19 @dd32
11 years ago

  • Milestone changed from Future Release to 3.7
  • Owner set to dd32
  • Status changed from new to accepted

#20 @dd32
11 years ago

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

In 25396:

Fix wp_guess_url() to work in every scenario I could find, allows us to use it to determine the correct path to the WordPress Site URL before installation for install.php and setup-config.php redirects. Fixes #24480 Fixes #16884

Note: See TracTickets for help on using tickets.