Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#21580 closed defect (bug) (wontfix)

check_and_publish_future_post() should set global $post

Reported by: mattyrob's profile MattyRob Owned by:
Milestone: Priority: normal
Severity: major Version: 3.4.1
Component: Cron API Keywords:
Focuses: Cc:

Description

The above function check to see if posts with a 'future' post type are due to become visible on a blog and triggers a bunch of hooks like the post transition hooks.

Hooks such as draft_to_publish behave differently from future_to_publish because certain variables that are global in the former case are not in the latter case.

Declaring the $post variable as a global in the check_and_publish_future_post() function resolves some of these differences, if possible perhaps the $post_id global should be declared somewhere too but I can't figure out where just yet.

Attachments (2)

21580.diff (432 bytes) - added by MattyRob 12 years ago.
21580v2.diff (487 bytes) - added by MattyRob 12 years ago.

Download all attachments as: .zip

Change History (15)

@MattyRob
12 years ago

#1 @scribu
12 years ago

Hooks such as draft_to_publish behave differently from future_to_publish because certain variables that are global in the former case are not in the latter case.

Please be more specific.

#2 @MattyRob
12 years ago

I'm not 100% sure what additional specifics you want but let me try and explain again a little more clearly.

WordPress has several 'transition' hooks that are called when a post or page are 'transitioned' from one publication state to another. I think there should be some consistency to the global variables that are available.

I am giving an example here that when the draft_to_publish hook is called the global variables of $post and $post_id are available to plugins however when hooking to the virtually identical future_to_publish hook these variables are not available.

The code below is a proof of concept that demonstrates this. The attached revised patch makes both $post and $post_id available as global variables to plugins.

function transition_hook() {
	global $post, $post_id;
	$email = 'your@email.com';
	$message = "Transition Hook was called with $post->ID and $post_id";
	@wp_mail($email, 'Transition Hook Plugin', $message);
}

add_action('draft_to_publish', 'transition_hook');
add_action('future_to_publish', 'transition_hook');

@MattyRob
12 years ago

#3 @MattyRob
12 years ago

  • Keywords dev-feedback added

#4 @scribu
12 years ago

  • Keywords dev-feedback removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Right; my thought was that plugins shouldn't rely on the globals at all, since they receive that information as parameters:

function transition_hook( $post_id, $post ) {
	$email = 'your@email.com';
	$message = "Transition Hook was called with $post->ID and $post_id";
	@wp_mail($email, 'Transition Hook Plugin', $message);
}

add_action('draft_to_publish', 'transition_hook', 10, 2);
add_action('future_to_publish', 'transition_hook', 10, 2);

The fact that the $post global happens to be the same in the 'draft_to_publish' hook is a coincidence. Notice that wp_transition_post_status() doesn't explicitly set the post global either:

function wp_transition_post_status($new_status, $old_status, $post) {
	do_action('transition_post_status', $new_status, $old_status, $post);
	do_action("{$old_status}_to_{$new_status}", $post);
	do_action("{$new_status}_{$post->post_type}", $post->ID, $post);
}

Given that 1) the patch might cause other unwanted side-effects and 2) we want to discourage use of globals in general, I'm closing this.

Last edited 12 years ago by scribu (previous) (diff)

#5 @MattyRob
12 years ago

  • Keywords has-patch removed

Plugins receive certain information as parameters ONLY if it is specified as part of the hook. Other parameters may be available if they are defined in previous code as a global. Clearly in the case of the draft_to_publish hook these items are already being defined.

You might want to update the codex about globals then and make it clear to developers to not use them, I've been developing plugins for 6 years and have never seen anything discouraging use of globals. In fact quite the opposite is my interpretation of this sentence at the top of the globals page.

"WordPress-specific global variables are used throughout WordPress code for various reasons. Almost all data that WordPress generates can be found in a global variable, which makes them very handy and important for developers."

Ticket left as closed though as I expect it's a waste of time re-opening.

#6 @scribu
12 years ago

Please paste the url to the Codex page you quoted from.

#7 follow-up: @MattyRob
12 years ago

Here you go:

http://codex.wordpress.org/Global_Variables

It does read to me as encouraging use of the variables.

From an efficiency perspective too is it not better to access variables like post rather than querying the database again?

#8 follow-up: @MattyRob
12 years ago

As a supplemental, I tried the code you pasted above and it throws an error:

Warning: Missing argument 2 for transition_hook() in /Users/Matthew/Sites/trunk/wp-content/plugins/transition_checker.php on line 8

I suspect this is because the function is expecting 2 parameters but the action from old to new status only passes 1 variable - namely $post.

#9 @nacin
12 years ago

So then instead of doing transition_hook() with global $post, just do transition_hook( $post ).

scribu and I are going to work on kill off that Codex page. Always try to avoid them if possible, especially if a function is already passing them to you.

Accessing $post as it gets passed to a function does not query them again. Calling get_post() does not even query the DB a second time — it uses what is in cache.

#10 in reply to: ↑ 7 @scribu
12 years ago

We didn't kill the page, but we removed the globals that shouldn't be accessed directly:

http://codex.wordpress.org/Global_Variables

Indeed, the old version of the Codex page encouraged unwaranted use of global variables. I didn't even know it existed, so thanks for bringing it up, MattyRob.

#11 in reply to: ↑ 8 @scribu
12 years ago

Replying to MattyRob:

As a supplemental, I tried the code you pasted above and it throws an error:

Warning: Missing argument 2 for transition_hook() in /Users/Matthew/Sites/trunk/wp-content/plugins/transition_checker.php on line 8

I suspect this is because the function is expecting 2 parameters but the action from old to new status only passes 1 variable - namely $post.

Yes, you need to tell add_filter how many parameters you want back:

add_action('draft_to_publish', 'transition_hook', 10, 2);
add_action('future_to_publish', 'transition_hook', 10, 2);
Version 1, edited 12 years ago by scribu (previous) (next) (diff)

#12 @MattyRob
12 years ago

Thanks for updating the Codex and I'm glad to hear avoiding use of globals isn't going to make my code database heavy.

@Nacin, I reported this because of an issue raised with me around plugin conflict, the other plugin takes no parameters and is beyond my control so maybe I should decline the request as a wontfix too!

@Scribu, I copied your code above exactly including those add action lines - still doesn't work. The do_action line being hooked to (do_action("{$old_status}_to_{$new_status}", $post);) only passes $post.

#13 @scribu
12 years ago

Yeah, I guess I was looking at do_action("{$new_status}_{$post->post_type}", $post->ID, $post);

Note: See TracTickets for help on using tickets.