PHP notices when publishing with QuickPress

[caesarsgrunt]

See attached screenshot taken after posting from QuickPress. There are also errors when saving a draft from QuickPress; though less of them.

The post is published anyway.

WP r12462, PHP 5.2.5

[westi]

If the post is published correctly then by default (i.e. with WP_DEBUG not enabled) these notices won't show and the 'expected' behaviour will be achieved.

Therefore this is not a severe enough issue to fix in a 2.9.1 release - moving to the 3.0 milestone where we will address it.

[hakre]

Westi, please defined "published correctly" in this ticket's report context. Please provide the steps which drove you into this conclusion. Looks pretty much to unchecked indezies to me.

Referenced: #10758

[nacin]

It's not a security issue or a blocker-level bug.

[westi]

Replying to hakre:

Westi, please defined "published correctly" in this ticket's report context. Please provide the steps which drove you into this conclusion. Looks pretty much to unchecked indezies to me.

Referenced: #10758

My understanding of this ticket was that the post published fine but if WP_DEBUG was enabled then you see those notices.

If the functionality works correctly but there are notices shown with WP_DEBUG enabled while the notices are not desirable and we would fix them for the next major release I would advocate against making the changes in a point release unless the notices cause a security/blocker bug

[hakre]

I'm pretty shure that I would have run over this as well in #10758 if Quickpress would have been used on the blog that was providing the data on unchecked indezies / falsly referenced variables. Since we already put focus of such warnings to get them fixed in 2.9 the consequence should be to fix the missed ones as well. Maybe not 2.9.1 but while there is no 2.9.2 milestone yet, I prefer 2.9.1 over 3.0. We can shift tickets with a not-high prio (like this one with normal) then to the next milestone (2.9.2) when 2.9.1 is released.

I will now try reproduce this. The point to WP_DEBUG is misleading because core code must run without any warning so that WP_DEBUG is a useable feature.

[hakre]

Notices Patch

[hakre]

I can confirm the problem, warnings because of unset variables while using Quickpress. Patch does fix the problem.

[caesarsgrunt]

Is there any reason not to fix this in 2.9, regardless of whether it is a blocker or not? It is still a bugfix and it has a patch.

(Not sure exactly what the criteria for eligibility for fixing in a point release is - I thought any bug could be fixed, but no new features. Is that wrong?)

[scribu]

Replying to caesarsgrunt:

Is there any reason not to fix this in 2.9, regardless of whether it is a blocker or not? It is still a bugfix and it has a patch.

(Not sure exactly what the criteria for eligibility for fixing in a point release is - I thought any bug could be fixed, but no new features. Is that wrong?)

Read the ​milestone description:

"This Milestone is only for security and blocker level bugs that exist in 2.9 after it is released."

[hakre]

Thanks for clarification. Please apply it to the current head branch which will become 3.0.

[nacin]

hakre:

I was using the patch when testing QuickPress for #10680. Looks good -- two things:

The line that sets $previous_status would now have two ternary operations, all for a function's second argument. It would probably be better as multiple lines.

Also,

if ( 'page' == ( isset($_POST['post_type']) ? $_POST['post_type'] : null ) )

Should probably be:

if ( isset($_POST['post_type']) && 'page' == $_POST['post_type'] )

That's all I got.

[nacin]

Ok.

QuickPress will never pass $user_ID, but setting it to null is not correct. Root of the problem: it should instead set it to $GLOBALSuser_ID?, or pass it via POST to begin with.

Patching.

[hakre]

I only set to NULL because that's the same value as if you read an unset variable by accident. So it does not change the behaviour compared to pre-patched. That's all, I know it's a bit akward.

Just reviewed your patch, why have you removed the cast to int in front of $post_data['user_ID']; Line 40 / 43?

Thanks for your review as well, I appreceate the feedback.

[nacin]

Good call, the int cast should not have been removed.

[nacin]

int cast added back in, also refreshed against trunk r12602

[Denis-de-Bernardy]

@nacin: isn't it possible for $post_data[ID] to be set with a value of zero on drafts? I'd suggest !empty() instead of isset() for the checks when setting the post_id variable.

[nacin]

Replying to Denis-de-Bernardy:

isn't it possible for $post_data[ID] to be set with a value of zero on drafts? I'd suggest !empty() instead of isset() for the checks when setting the post_id variable.

Not sure. The code was already checking for isset(). Logic seems fine though for $post_id to be 0, I think. It's just using that to get the post's previous status.

[ryan]

(In [12873]) Fix notices when publishing with QuickPress. Props nacin, hakre. fixes #11504