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

[kellenmace]

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.

[kellenmace]

icon-functions.php

[dd32]

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]

icon-functions.php

[kellenmace]

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

[sami.keijonen]

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.

[thamaraiselvam]

@readfile()

could be better because it will suppress error.

[sami.keijonen]

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.

[sabernhardt]

refreshing the patch

[bedas]

Any reason why to bypass the WPCS and not use

<?php
require_once ABSPATH . 'wp-admin/includes/class-wp-filesystem-base.php';
require_once ABSPATH . 'wp-admin/includes/class-wp-filesystem-direct.php';
$filesystem = new WP_Filesystem_Direct( true );
$path = //define path to whatever file

$filesystem->get_contents($path)

?

Admittedly much more cumbersome than a one-function call, but it is what WPCS expects us to do.