Make WordPress Core

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's profile 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)

icon-functions.diff (410 bytes) - added by kellenmace 8 years ago.
icon-functions.php
icon-functions2.diff (396 bytes) - added by kellenmace 8 years ago.
icon-functions.php
40292.diff (524 bytes) - added by sabernhardt 20 months ago.
refreshing the patch

Download all attachments as: .zip

Change History (14)

@kellenmace
8 years ago

icon-functions.php

#1 @dd32
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?

@kellenmace
8 years ago

icon-functions.php

#2 @kellenmace
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 @sami.keijonen
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.

#4 @thamaraiselvam
8 years ago

@readfile()

could be better because it will suppress error.

#5 @SergeyBiryukov
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 @sami.keijonen
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.

Last edited 8 years ago by sami.keijonen (previous) (diff)

@sabernhardt
20 months ago

refreshing the patch

#7 @sabernhardt
20 months ago

  • Keywords has-patch added

#8 @anonymized_14808221
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.

Last edited 14 months ago by anonymized_14808221 (previous) (diff)

#9 @karmatosed
7 months ago

  • Keywords dev-feedback added

#10 @karmatosed
7 months ago

  • Milestone changed from Awaiting Review to Future Release

#11 @karmatosed
3 months ago

  • Keywords dev-feedback removed
Note: See TracTickets for help on using tickets.