Make WordPress Core

Opened 14 years ago

Closed 14 years ago

#11504 closed defect (bug) (fixed)

PHP notices when publishing with QuickPress

Reported by: caesarsgrunt's profile caesarsgrunt Owned by: westi's profile westi
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: Warnings/Notices Keywords: has-patch needs-testing
Focuses: Cc:

Description

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

Attachments (4)

Screen shot 2009-12-19 at 13.44.27.png (60.7 KB) - added by caesarsgrunt 14 years ago.
11504.patch (2.3 KB) - added by hakre 14 years ago.
Notices Patch
11504.3.patch (2.8 KB) - added by nacin 14 years ago.
11504.4.diff (2.5 KB) - added by nacin 14 years ago.
int cast added back in, also refreshed against trunk r12602

Download all attachments as: .zip

Change History (22)

#1 @scribu
14 years ago

  • Summary changed from PHP errors when publishing with QuickPress to PHP notices when publishing with QuickPress

#2 @westi
14 years ago

  • Milestone changed from 2.9.1 to 3.0
  • Status changed from new to accepted

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.

#3 follow-up: @hakre
14 years ago

  • Milestone changed from 3.0 to 2.9.1

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

#4 @nacin
14 years ago

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

#5 in reply to: ↑ 3 @westi
14 years ago

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

#6 @hakre
14 years ago

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
14 years ago

Notices Patch

#7 @hakre
14 years ago

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

#8 @hakre
14 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#9 follow-up: @caesarsgrunt
14 years ago

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?)

#10 in reply to: ↑ 9 @scribu
14 years ago

  • Milestone changed from 2.9.1 to 3.0

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."

#11 @hakre
14 years ago

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

#12 @nacin
14 years ago

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.

#13 @nacin
14 years ago

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.

@nacin
14 years ago

#14 @hakre
14 years ago

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.

#15 @nacin
14 years ago

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

@nacin
14 years ago

int cast added back in, also refreshed against trunk r12602

#16 follow-up: @Denis-de-Bernardy
14 years ago

@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.

#17 in reply to: ↑ 16 @nacin
14 years ago

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.

#18 @ryan
14 years ago

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

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

Note: See TracTickets for help on using tickets.