Opened 8 years ago
Last modified 3 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: | Future Release | Priority: | normal |
Severity: | normal | Version: | 4.7.3 |
Component: | Bundled Theme | Keywords: | has-patch 2nd-opinion |
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 (3)
Change History (14)
#1
@
8 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?
#2
@
8 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
@
8 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.
#5
@
8 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
@
8 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
.
#8
@
14 months ago
- Keywords 2nd-opinion added
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.
icon-functions.php