WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 years 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:
PR Number:

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 3 years ago.
icon-functions.php
icon-functions2.diff (396 bytes) - added by kellenmace 3 years ago.
icon-functions.php

Download all attachments as: .zip

Change History (8)

@kellenmace
3 years ago

icon-functions.php

#1 @dd32
3 years 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
3 years ago

icon-functions.php

#2 @kellenmace
3 years 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
3 years 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
3 years ago

@readfile()

could be better because it will suppress error.

#5 @SergeyBiryukov
3 years 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
3 years 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 3 years ago by sami.keijonen (previous) (diff)
Note: See TracTickets for help on using tickets.