WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 10 months ago

#40292 new defect (bug)

Twenty Seventeen: Use echo file_get_contents() instead of require_once() to pull in SVG file contents

Reported by: kellenmace Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.7.3
Component: Bundled Theme Keywords:
Focuses: Cc:

Description

Using require_once() to pull in the contents of SVG files can result in the PHP parser throwing a

PHP Parse error: syntax error, unexpected version (T_STRING)

error if any of the SVG files begin with

<?xml version="1.0" encoding="UTF-8"?>

The proposed solution is to use echo file_get_contents() instead.

A few recommendations for using that method are here:
https://css-tricks.com/using-svg/#article-header-id-7
http://sheelahb.com/blog/how-to-get-php-to-play-nicely-with-svg-files/

It could be argued that using require_once() is fine in Twenty Seventeen, since we know that none of the SVGs in /assets/images/svg-icons.svg contain the problematic <?xml … ?> tag. However, there are of course many developers who fork Twenty Seventeen, or copy its code into their own themes, so it seems wise to me to pull in SVG file contents using a method that won't throw errors in the event that <?xml … ?> tags are present in any SVG files.

Attachments (2)

icon-functions.diff (410 bytes) - added by kellenmace 10 months ago.
icon-functions.php
icon-functions2.diff (396 bytes) - added by kellenmace 10 months ago.
icon-functions.php

Download all attachments as: .zip

Change History (8)

@kellenmace
10 months ago

icon-functions.php

#1 @dd32
10 months ago

I'd probably suggest readfile() over file_get_contents() as it doesn't cause it to be copied to memory before being output.
Avoiding include()/require() for non-PHP files seems like a sane suggestion. This currently uses require_once(), but I can't imagine it'd get called twice on a page?

@kellenmace
10 months ago

icon-functions.php

#2 @kellenmace
10 months ago

@dd32 - Thanks for the recommendation to use readfile() to avoid reading the file into memory, then echoing it. I have uploaded icon-functions2.diff​ that uses readfile() instead.

#3 @sami.keijonen
10 months ago

Thanks @kellenmace for report. It was my suggestion originally to use require_once. I did know about the possible error if the SVG file starts with <?xml version="1.0" encoding="UTF-8"?>.

In this case it doesn't.

With that said I don't have objection for using readfile() if it's safe, fast, and reliable method.

#4 @thamaraiselvam
10 months ago

@readfile()

could be better because it will suppress error.

#5 @SergeyBiryukov
10 months ago

  • Summary changed from Use echo file_get_contents() instead of require_once() to pull in SVG file contents in Twenty Seventeen to Twenty Seventeen: Use echo file_get_contents() instead of require_once() to pull in SVG file contents

#6 @sami.keijonen
10 months ago

could be better because it will suppress error.

Just noting that there is no error in Twenty17. But because code probably get copied a lot without thinking, readfile() makes sense if in other themes they are using <?xml.

Last edited 10 months ago by sami.keijonen (previous) (diff)
Note: See TracTickets for help on using tickets.