WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#21580 closed defect (bug) (wontfix)

check_and_publish_future_post() should set global $post

Reported by: 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 20 months ago.
21580v2.diff (487 bytes) - added by MattyRob 20 months ago.

Download all attachments as: .zip

Change History (15)

MattyRob20 months ago

comment:1 scribu20 months 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.

comment:2 MattyRob20 months 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');

MattyRob20 months ago

comment:3 MattyRob20 months ago

  • Keywords dev-feedback added

comment:4 scribu20 months 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 20 months ago by scribu (previous) (diff)

comment:5 MattyRob20 months 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.

comment:6 scribu20 months ago

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

comment:7 follow-up: MattyRob20 months 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?

comment:8 follow-up: MattyRob20 months 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.

comment:9 nacin20 months 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.

comment:10 in reply to: ↑ 7 scribu20 months 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.

comment:11 in reply to: ↑ 8 scribu20 months 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_action() how many parameters you want back:

add_action('draft_to_publish', 'transition_hook', 10, 2);
add_action('future_to_publish', 'transition_hook', 10, 2);
Last edited 20 months ago by scribu (previous) (diff)

comment:12 MattyRob20 months 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.

comment:13 scribu20 months 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.