WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#3996 closed enhancement (fixed)

MAGPIE_USER_AGENT lack of wp version

Reported by: hakre Owned by: westi
Milestone: 2.2.3 Priority: normal
Severity: normal Version: 2.1.2
Component: Template Keywords: rss magpie snoopy has-patch commit
Focuses: Cc:

Description

Due to an error, the global variable $wp_version is not imported into the local namespace of the init function within wp-includes/rss.php (~ line 565). It is used ~ line 594 where the MAGPIE_USER_AGENT is defined (if not already defined before ~ line 12). Because the variable does not exists in the namespace there, it should changed to $GLOBALS['wp_version'].

change from:
		$ua = 'WordPress/' . $wp_version;
change to:
		$ua = 'WordPress/' . $GLOBALS['wp_version'];

this is an error by design. let's face it, the real mess starts a lot earlier in line 12:

define('MAGPIE_USER_AGENT', 'WordPress/' . $wp_version);

this is the place where the constant is defined first. at this time $wp_version is not set (version.php not included). a workaround would be to ask for existance of $wp_version before defining the constant:

if (isset($wp_version)) define('MAGPIE_USER_AGENT', 'WordPress/' . $wp_version);

but this might create overhead and other problems as well because it might no be defined when it is assumed that it is. therefore i suggest to include version.php before including rss.php since rss.php is based on version.php.

version.php is included in wp-settings.php on line 171

rss.php is included in my case in a widget within my template. since all the template related stuff is loaded via template loader in wp-blog-header.php, this results in being in index.php on line 4.

I'm quite new to wordpress development, but shouldn't it make sense to include version.php in index.php already? or should rss.php have a include_once('version.php') inside? whatever in my case the isset() option describben above did the job for me quite well.

Attachments (1)

3996.diff (614 bytes) - added by Nazgul 8 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 @foolswisdom8 years ago

  • Milestone changed from 2.1.3 to 2.2

comment:2 follow-up: @JeremyVisser8 years ago

  • Milestone 2.2 deleted
  • Resolution set to invalid
  • Status changed from new to closed

Actually, I think you must be jumping the loop or something, because I just added a line like this to my rss.php directly after that define() statement:

error_log("wp_version is $wp_version");

and the version was outputted as expected to the Apache log. Works fine for me.

I think you probably shouldn't be including rss.php manually, or something like that. Have a look at how the dashboard does it, as a point of reference.

comment:3 @hakre8 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Hi JeremyVisser, thanks for taking the time to look into the issue. Unfourtionatly you should have read what I was reporting, especially the following:

rss.php is included in my case in a widget within my template. since all the
template related stuff is loaded via template loader in wp-blog-header.php, this
results in being in index.php on line 4.

As you can sea, your way testing is testing nothing at all. since you have not tested what I was writing about nor taking the time to catch on my questions I re-open the issue since it is wether solved nor invalid.

comment:4 @foolswisdom8 years ago

  • Milestone set to 2.4 (future)

comment:5 @Otto428 years ago

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

I'm also unable to reproduce your problem.

hakre: I think the issue is that you are wrong about the ordering of the files loading.

wp-blog-header.php loads wp-config.php which then loads wp-settings.php. After all this is done, and you make it back to wp-blog-header.php, then it goes into the template-loader.php.

wp-settings.php (and thus version.php) is loaded well before anything in the theme is. What's more, version.php is loaded before any plugins, before the my-hacks file, before the theme's functions.php file... basically before any normally user-modifiable code.

In order for what you're saying to be true, you'd have to include rss.php well before any of that stuff, but you're saying that you're loading it via "a widget within my template" which doesn't work. wp-version is already set by that point.

Now, you are correct that the function init() in rss.php is busted because it does not declare $wp_version as global, however the define at the top of the rss.php file prevents that from making any difference, since it prevents that code from executing anyway.

Anyway, unless you can show a specific case that we can duplicate where $wp_version is not set by the time you require_once("rss.php"), please don't reopen this ticket.

comment:6 @Nazgul8 years ago

  • Milestone 2.4 (future) deleted

comment:7 @westi8 years ago

  • Component changed from Administration to Template
  • Keywords needs-patch added
  • Milestone set to 2.2.2
  • Resolution invalid deleted
  • Status changed from closed to reopened

I understand this issue.

The fact is that rss.php is included in a number of places:

http://trac.wordpress.org/browser/trunk/wp-includes/widgets.php#L935
http://trac.wordpress.org/browser/trunk/wp-includes/widgets.php#L1006
http://trac.wordpress.org/browser/trunk/wp-admin/index-extra.php#L3

Only the last of these is at global scope.

Therefore in the two widgets cases $wp_version will not be defined - php requires and includes get the scope of the line of code on which they occur.

We therefore need to fix the implicit use of globals in rss.php or include it at global scope in widgets.php

comment:8 @rob1n8 years ago

Can we replace those with $GLOBALSfoo?? Would that work?

(I'm talking in rss.php)

comment:9 @Otto428 years ago

If he's correct, then using $GLOBALS['wp_version']; in both spots should work. version.php is definitely loaded before the widgets are, so the global $wp_version should be defined. From what westi says, it's mostly just a matter of rss.php being loaded outside of the global scope.

comment:10 @rob1n8 years ago

Makes sense, so we can just move to using $GLOBALS[] in rss.php, so no matter how the rss.php file was included (what scope), it will always have that $GLOBALS[] variable.

@Nazgul8 years ago

comment:11 @Nazgul8 years ago

  • Keywords has-patch added; needs-patch removed

Patch based on comments added.

comment:12 @westi8 years ago

  • Owner changed from anonymous to westi
  • Status changed from reopened to new

That patch looks good.

comment:13 in reply to: ↑ 2 @hakre8 years ago

the patch looks good for me as well. thanks Nazgul!

comment:14 @Nazgul8 years ago

  • Keywords commit added
  • Milestone changed from 2.2.2 to 2.2.3

comment:15 follow-up: @markjaquith8 years ago

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

(In [5859]) globalize wp_version so Magpie can use it. props Nazgul, hakre. fixes #3996

comment:16 in reply to: ↑ 15 ; follow-ups: @westi8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to markjaquith:

(In [5859]) globalize wp_version so Magpie can use it. props Nazgul, hakre. fixes #3996

Cheers for checking that in mark.

This was targeted for branches/2.2 as well so if you could fix it there too in case we do another 2.2.x release that would be cool.

comment:17 in reply to: ↑ 16 @hakre8 years ago

Replying to westi:

This was targeted for branches/2.2 as well so if you could fix it there too in case we do another 2.2.x release that would be cool.

How can that be done? Or can this be only done by specific users?

comment:18 in reply to: ↑ 16 @hakre8 years ago

Replying to westi:

Replying to markjaquith:

(In [5859]) globalize wp_version so Magpie can use it. props Nazgul, hakre. fixes #3996

Cheers for checking that in mark.

This was targeted for branches/2.2 as well so if you could fix it there too in case we do another 2.2.x release that would be cool.

I would do that but I dunno how. Should I download 2.2.2 and do a patch against it?

comment:19 @westi8 years ago

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

(In [5961]) Globalise wp_version so Magpie can use it. props Nazgul, hakre. Fixes #3996 for 2.2.3

Note: See TracTickets for help on using tickets.