Skip to content

BUGFIX: Files::getFileContents broken in PHP 7.1#821

Merged
kitsunet merged 3 commits intoneos:4.0from
dfeyer:bugfix-files-php71
Jan 26, 2017
Merged

BUGFIX: Files::getFileContents broken in PHP 7.1#821
kitsunet merged 3 commits intoneos:4.0from
dfeyer:bugfix-files-php71

Conversation

@dfeyer
Copy link
Copy Markdown
Contributor

@dfeyer dfeyer commented Jan 12, 2017

This change set the offset to 0 (default value in file_get_contents.)

PHP 7.1 changed the behaviour of file_get_contents regarding
negative offset (support for negative offsets has been added.)

Without this change the sites Root.fusion is not loaded, so that
Files::getFileContents return an empty string. Tested on PHP 7.1.0
and 7.0.12 (worked without this fix).

This change set the offset to 0 (default value in file_get_contents.
PHP 7.1 change the behaviour of file_get_contents regarding
negative offset for many core functions.

Without this change the site Root.fusion is not loaded,
Files:getFileContents return an empty string. Tested on PHP 7.1.0.
@mention-bot
Copy link
Copy Markdown

@dfeyer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @daniellienert, @kdambekalns and @kitsunet to be potential reviewers.

@dfeyer
Copy link
Copy Markdown
Contributor Author

dfeyer commented Jan 12, 2017

Please merge this one before #816

@dfeyer dfeyer changed the title BUGFIX: Files:getFileContents broken in PHP7 BUGFIX: Files:getFileContents broken in PHP7.1 Jan 12, 2017
@kitsunet
Copy link
Copy Markdown
Member

Looks good so far, shouldn't we also change the default to null instead of -1 then?

@kdambekalns kdambekalns changed the title BUGFIX: Files:getFileContents broken in PHP7.1 BUGFIX: Files::getFileContents broken in PHP 7.1 Jan 13, 2017
* @param integer $offset (optional) Offset where reading of the file starts.
* @param integer $maximumLength (optional) Maximum length to read. Default is -1 (no limit)
* @return mixed The file content as a string or FALSE if the file could not be opened.
* @throws FilesException
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Erm, it doesn't throw anything, right? The exception is caught in line 324…

* @throws FilesException
* @api
*/
public static function getFileContents($pathAndFilename, $flags = 0, $context = null, $offset = -1, $maximumLength = -1)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$offset should be changed to null, I think (too).

@@ -306,6 +306,7 @@ public static function copyDirectoryRecursively($sourceDirectory, $targetDirecto
* @param integer $offset (optional) Offset where reading of the file starts.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add hint that on PHP 7.1 negative offset is supported?

@kdambekalns
Copy link
Copy Markdown
Member

I adjusted this a bit, so now the default is null and the check for -1 is gone. Otherwise that would never have worked, in the (unlikely?) case of being needed.

Copy link
Copy Markdown
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now… did not test, though (sorry!)

@kdambekalns kdambekalns added this to the 4.0 milestone Jan 26, 2017
@kitsunet kitsunet merged commit 5bbab56 into neos:4.0 Jan 26, 2017
@kitsunet
Copy link
Copy Markdown
Member

kitsunet commented Jan 29, 2017

ok, apparently this is the fix for the > bug in setup/login that people are talking about in slack (https://neos-project.slack.com/archives/neos-general/p1485696935005590) so apparently we need to backport to lower branches as well :/
And SOON, because it'S unusable with PHP 7.1.

@kdambekalns
Copy link
Copy Markdown
Member

@kitsunet Why backport? Does everything else work with 7.1? Then I'd say yes. But we promise 7.1 compatibility starting with 4.0 only, so otherwise…

@kitsunet
Copy link
Copy Markdown
Member

kitsunet commented Jan 30, 2017

right, to be checked, probably not, but we should add respective php requirements then. currently you can install flow 3.x / neos 2.x with php 7.1, it just doesn't work.

@kdambekalns
Copy link
Copy Markdown
Member

Ok, if that is the only thing breaking it on 7.1 for older releases, we should backport. If not fix the constraint on PHP -> see #847

@bwaidelich
Copy link
Copy Markdown
Member

This also fixes PHP 7.1 support for Flow 3.3 / Neos 2.3 - I'll take care of the backport

bwaidelich added a commit to bwaidelich/flow-development-collection that referenced this pull request Apr 12, 2017
With PHP 7.1 the behavior of `file_get_contents()` has been changed
allowing negative offsets to be specified.

This change adjusts the signature of `TYPO3\Flow\Utility\Files::getFileContents()`
accordingly.

*Note:* This is a backport of neos#821 which has only been applied to the neos:4.0 branch

Fixes: neos#1301
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants