Ticket #3996 (closed enhancement: fixed)

Opened 2 years ago

Last modified 1 year ago

MAGPIE_USER_AGENT lack of wp version

Reported by: hakre Assigned to: westi
Priority: normal Milestone: 2.2.3
Component: Template Version: 2.1.2
Severity: normal Keywords: rss magpie snoopy has-patch commit
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

3996.diff (0.6 kB) - added by Nazgul on 07/21/07 16:17:41.

Change History

03/19/07 00:29:47 changed by foolswisdom

  • milestone changed from 2.1.3 to 2.2.

(follow-up: ↓ 13 ) 03/19/07 03:48:15 changed by JeremyVisser

  • status changed from new to closed.
  • resolution set to invalid.
  • milestone deleted.

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.

06/18/07 13:31:26 changed by hakre

  • status changed from closed to reopened.
  • resolution deleted.

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.

06/18/07 14:23:27 changed by foolswisdom

  • milestone set to 2.4 (future).

06/18/07 17:23:34 changed by Otto42

  • status changed from reopened to closed.
  • resolution set to invalid.

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.

06/18/07 20:34:07 changed by Nazgul

  • milestone deleted.

06/18/07 21:27:42 changed by westi

  • keywords changed from rss magpie snoopy to rss magpie snoopy needs-patch.
  • resolution deleted.
  • status changed from closed to reopened.
  • component changed from Administration to Template.
  • milestone set to 2.2.2.

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

06/19/07 17:12:27 changed by rob1n

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

(I'm talking in rss.php)

06/19/07 19:16:13 changed by Otto42

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.

06/20/07 04:49:22 changed by rob1n

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.

07/21/07 16:17:41 changed by Nazgul

  • attachment 3996.diff added.

07/21/07 16:18:24 changed by Nazgul

  • keywords changed from rss magpie snoopy needs-patch to rss magpie snoopy has-patch.

Patch based on comments added.

07/22/07 20:55:54 changed by westi

  • owner changed from anonymous to westi.
  • status changed from reopened to new.

That patch looks good.

(in reply to: ↑ 2 ) 08/09/07 11:06:43 changed by hakre

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

08/09/07 15:40:57 changed by Nazgul

  • keywords changed from rss magpie snoopy has-patch to rss magpie snoopy has-patch commit.
  • milestone changed from 2.2.2 to 2.2.3.

(follow-up: ↓ 16 ) 08/10/07 08:07:53 changed by markjaquith

  • status changed from new to closed.
  • resolution set to fixed.

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

(in reply to: ↑ 15 ; follow-ups: ↓ 17 ↓ 18 ) 08/10/07 09:05:07 changed by westi

  • status changed from closed to reopened.
  • resolution deleted.

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.

(in reply to: ↑ 16 ) 08/10/07 10:26:50 changed by hakre

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?

(in reply to: ↑ 16 ) 08/23/07 23:30:27 changed by hakre

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?

08/28/07 20:21:40 changed by westi

  • status changed from reopened to closed.
  • resolution set to fixed.

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