WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#17197 closed defect (bug) (maybelater)

Attach media - search results not populating

Reported by: tomauger Owned by:
Milestone: Priority: normal
Severity: minor Version: 3.1
Component: General Keywords: 2nd-opinion
Focuses: Cc:

Description

Issue: Page / Post search not populating when using the Attach feature from the Media Library.

Details: upon closer inspection this occurs as a result of extra whitespace in functions.php (ie: outside the <?php ?> tags

To reproduce using TwentyTen:

  1. edit functions.php
  2. close the php code block with ?> at the very end of functions.php (why is it even left open??)
  3. add some blank lines (CR/LF) to the end of functions.php after the ?>

Reload Media Library and attempt to use the Search feature: results are blank.

Alternative steps:

  1. edit functions.php
  2. add whitespace before the <?php opening tag

Why I think this is a bug:
While it's not necessarily a good practice to add whitespace outside of the <?php ?> tags, it happens. More likely it will happen inside any child theme's functions.php, particularly if users are glibly copying-and-pasting code blocks from various sources on the interwebs. It seems strange that whatever code is displaying the search results even cares about whitespace in functions.php, but it would seem to me that it should not be displaying the contents of functions.php in any case, so the whitespace ought not to make a difference.

Change History (11)

comment:1 in reply to: ↑ description duck_3 years ago

Replying to tomauger:

(why is it even left open??)

To stop stuff like this from happening since a theme's functions.php is one of the files that is commonly edited by users and we don't want them to inadvertently add the whitespace outside of the tags.

The whitespace is probably causing a headers already sent error which is giving you trouble. Stuff outside of tags isn't parsed by PHP and is sent to the browser.

comment:2 tomauger3 years ago

  • Keywords reporter-feedback added

Fair enough. On the theme's built-in functions.php this seems like a reasonable precaution, but there's still a risk there, for users building their own child-of-twentyten/functions.php.

comment:3 tomauger3 years ago

  • Keywords reporter-feedback removed

(edit - was using reporter-feedback tag incorrectly)

comment:4 nacin3 years ago

There are other tickets suggesting that we do output buffering etc. I don't think that has enough support at this time.

Closing this one, as it's invalid: The issue is a user error. The first characters should be <?php and there's no need to close it. As duck_ said, we don't close PHP tags on files that are commonly edited, specifically functions.php and wp-config-(sample)?.php.

comment:5 nacin3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

comment:6 follow-up: tomauger3 years ago

  • Keywords 2nd-opinion added
  • Resolution invalid deleted
  • Status changed from closed to reopened

@nacin: not to be argumentative - obviously this is your call - but the issue I was reporting was less about the built in functions.php and whether it did or did not have a closing ?> tag. Though I appreciate your clarification on the reason behind leaving it out - makes sense!

The issue is that the Search dialog box of the Attach feature in the Media Library stops working the moment any whitespace creeps into the .php include files. It's a very likely scenario that this will occur, particularly in child themes where the user is building the php files often from scratch themselves.

The bottom line is: this Search feature is VERY likely to break and the reason is certainly not documented anywhere that's easily found (other than the one post on the forum where I documented my findings), so why not eliminate the issue?

If the issue is extra whitespace creeping into the output of the Search results (which TBH doesn't make sense to me, since HTML chomps whitespace ordinarily), then why not run a quick RegExp that trims that whitespace before outputting? (eg: s/\s* to trim leading whitespace). Just putting an idea out there.

Don't mark this as invalid - IMHO it's a valid bug because of the way the search results dialog was built - it's just low priority.

comment:7 in reply to: ↑ 6 duck_3 years ago

Replying to tomauger:

If the issue is extra whitespace creeping into the output of the Search results (which TBH doesn't make sense to me, since HTML chomps whitespace ordinarily), then why not run a quick RegExp that trims that whitespace before outputting? (eg: s/\s* to trim leading whitespace). Just putting an idea out there.

It seems that in this case the problem is that XML is being returned and this is indicated by a text/xml Content-Type header. Unfortunately the whitespace before the opening xml declaration means that jQuery throws an error when trying to parse it. This means that the error AJAX callback is used which tries to use the returned responseText which is the returned XML, but strips all the XML tags leaving a CDATA tag which is displayed by the browser but invisible.

We could check if textStatus == 'parseerror' and display a message in the error callback.

comment:8 aaroncampbell3 years ago

  • Keywords close added

Replying to tomauger:

The issue is that the Search dialog box of the Attach feature in the Media Library stops working the moment any whitespace creeps into the .php include files. It's a very likely scenario that this will occur, particularly in child themes where the user is building the php files often from scratch themselves.

You'll find that whitespace outside PHP tags will cause many problems (not limited to the search you're talking about). Often your RSS feed won't display properly in strict parsers (like FireFox), ajax calls can be a real pain (which is the issue here), etc, etc. The real problem here is that the whitespace shouldn't be there. WordPress can't deal with every programming mistake that someone might make. To me the added effort, cpu cycles, lines of code, etc (even though small) just isn't worth it here.

I recommend re-closing as invalid.

comment:9 follow-up: tomauger3 years ago

Thanks for the consideration. Your argument makes sense.

Question: would this error be flagged if running WP in debug mode? If so, then I definitely agree that the platform has done all it needs to in order to ensure a smooth 3rd party developer experience.

If not, then maybe _duck's solution could be a simple fix (there's not a lot of lines of code / cpu cycles for a simple error check and error log).

comment:10 westi3 years ago

  • Keywords close removed
  • Resolution set to maybelater
  • Status changed from reopened to closed

Closing as maybelater.

This and other issues can occur when you have extra white space output.

We do not intend on doing anything in core to fix this configuration issue at this time.

comment:11 in reply to: ↑ 9 SergeyBiryukov3 years ago

Replying to tomauger:

Question: would this error be flagged if running WP in debug mode? If so, then I definitely agree that the platform has done all it needs to in order to ensure a smooth 3rd party developer experience.


Extra whitespace in user-editable files generally causes "Cannot modify header information" warning, which is widely documented on the web, including Codex.

Note: See TracTickets for help on using tickets.