From 5bc4f3d1fc6567d9edb840a28503be30902d5680 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 1 Nov 2017 13:08:37 +1300 Subject: [PATCH 1/4] BUG Remove usage of deprecated each() and use a helper method instead --- src/Forms/GridField/GridField.php | 21 ++++++-- src/ORM/ArrayLib.php | 29 ++++++++++ src/ORM/Hierarchy/MarkedSet.php | 3 +- src/View/Parsers/Diff.php | 9 ++-- tests/php/ORM/ArrayLibTest.php | 89 +++++++++++++++++++++++++++++++ 5 files changed, 141 insertions(+), 10 deletions(-) diff --git a/src/Forms/GridField/GridField.php b/src/Forms/GridField/GridField.php index 94e335629..ebaeb6be7 100644 --- a/src/Forms/GridField/GridField.php +++ b/src/Forms/GridField/GridField.php @@ -105,6 +105,11 @@ class GridField extends FormField */ protected $name = ''; + /** + * Pattern used for looking up + */ + const FRAGMENT_REGEX = '/\$DefineFragment\(([a-z0-9\-_]+)\)/i'; + /** * @param string $name * @param string $title @@ -362,11 +367,19 @@ class GridField extends FormField 'before' => true, 'after' => true, ); + $fragmentDeferred = []; - reset($content); + // TODO: Break the below into separate reducer methods - while (list($contentKey, $contentValue) = each($content)) { - if (preg_match_all('/\$DefineFragment\(([a-z0-9\-_]+)\)/i', $contentValue, $matches)) { + // Continue looping if any placeholders exist + while (array_filter($content, function ($value) { + return preg_match(self::FRAGMENT_REGEX, $value); + })) { + foreach ($content as $contentKey => $contentValue) { + // Skip if this specific content has no placeholders + if (!preg_match_all(self::FRAGMENT_REGEX, $contentValue, $matches)) { + continue; + } foreach ($matches[1] as $match) { $fragmentName = strtolower($match); $fragmentDefined[$fragmentName] = true; @@ -380,7 +393,7 @@ class GridField extends FormField // If the fragment still has a fragment definition in it, when we should defer // this item until later. - if (preg_match('/\$DefineFragment\(([a-z0-9\-_]+)\)/i', $fragment, $matches)) { + if (preg_match(self::FRAGMENT_REGEX, $fragment, $matches)) { if (isset($fragmentDeferred[$contentKey]) && $fragmentDeferred[$contentKey] > 5) { throw new LogicException(sprintf( 'GridField HTML fragment "%s" and "%s" appear to have a circular dependency.', diff --git a/src/ORM/ArrayLib.php b/src/ORM/ArrayLib.php index 8df958d59..d47bc073b 100644 --- a/src/ORM/ArrayLib.php +++ b/src/ORM/ArrayLib.php @@ -2,6 +2,8 @@ namespace SilverStripe\ORM; +use Generator; + /** * Library of static methods for manipulating arrays. */ @@ -263,4 +265,31 @@ class ArrayLib return $out; } + + /** + * Iterate list, but allowing for modifications to the underlying list. + * Items in $list will only be iterated exactly once for each key, and supports + * items being removed or deleted. + * List must be associative. + * + * @param array $list + * @return Generator + */ + public static function iterateVolatile(array &$list) + { + // Keyed by already-iterated items + $iterated = []; + // Get all items not yet iterated + while ($items = array_diff_key($list, $iterated)) { + // Yield all results + foreach ($items as $key => $value) { + // Skip items removed by a prior step + if (array_key_exists($key, $list)) { + // Ensure we yield from the source list + $iterated[$key] = true; + yield $key => $list[$key]; + } + } + } + } } diff --git a/src/ORM/Hierarchy/MarkedSet.php b/src/ORM/Hierarchy/MarkedSet.php index 353b11cf8..96fd23602 100644 --- a/src/ORM/Hierarchy/MarkedSet.php +++ b/src/ORM/Hierarchy/MarkedSet.php @@ -5,6 +5,7 @@ namespace SilverStripe\ORM\Hierarchy; use InvalidArgumentException; use LogicException; use SilverStripe\Core\Injector\Injectable; +use SilverStripe\ORM\ArrayLib; use SilverStripe\ORM\ArrayList; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\FieldType\DBField; @@ -459,7 +460,7 @@ class MarkedSet // Build markedNodes for this subtree until we reach the threshold // foreach can't handle an ever-growing $nodes list - while (list(, $node) = each($this->markedNodes)) { + foreach (ArrayLib::iterateVolatile($this->markedNodes) as $node) { $children = $this->markChildren($node); if ($nodeCountThreshold && sizeof($this->markedNodes) > $nodeCountThreshold) { // Undo marking children as opened since they're lazy loaded diff --git a/src/View/Parsers/Diff.php b/src/View/Parsers/Diff.php index c32606fc3..2072e854e 100644 --- a/src/View/Parsers/Diff.php +++ b/src/View/Parsers/Diff.php @@ -181,16 +181,15 @@ class Diff extends \Diff $content = str_replace(array(" ", "<", ">"), array(" "," <", "> "), $content); $candidateChunks = preg_split("/[\t\r\n ]+/", $content); $chunks = []; - while ($chunk = each($candidateChunks)) { - $item = $chunk['value']; + for ($i = 0; $i < count($candidateChunks); $i++) { + $item = $candidateChunks[$i]; if (isset($item[0]) && $item[0] == "<") { $newChunk = $item; while ($item[strlen($item)-1] != ">") { - $chunk = each($candidateChunks); - if ($chunk === false) { + if (++$i >= count($candidateChunks)) { break; } - $item = $chunk['value']; + $item = $candidateChunks[$i]; $newChunk .= ' ' . $item; } $chunks[] = $newChunk; diff --git a/tests/php/ORM/ArrayLibTest.php b/tests/php/ORM/ArrayLibTest.php index b4bc00280..91460b747 100644 --- a/tests/php/ORM/ArrayLibTest.php +++ b/tests/php/ORM/ArrayLibTest.php @@ -246,4 +246,93 @@ class ArrayLibTest extends SapphireTest $this->assertEquals($expected, ArrayLib::flatten($options)); } + + /** + * Test that items can be added during iteration + */ + public function testIterateVolatileAppended() + { + $initial = [ + 'one' => [ 'next' => 'two', 'prev' => null ], + 'two' => [ 'next' => 'three', 'prev' => 'one' ], + 'three' => [ 'next' => null, 'prev' => 'two' ], + ]; + + // Test new items are iterated + $items = $initial; + $seen = []; + foreach (ArrayLib::iterateVolatile($items) as $key => $value) { + $seen[$key] = $value; + // Append four + if ($key === 'three') { + $items['three']['next'] = 'four'; + $items['four'] = [ 'next' => null, 'prev' => 'three']; + } + // Prepend zero won't force it to be iterated next, but it will be iterated + if ($key === 'one') { + $items['one']['next'] = 'zero'; + $items = array_merge( + ['zero' => [ 'next' => 'one', 'prev' => 'three']], + $items + ); + } + } + $expected = [ + 'one' => [ 'next' => 'two', 'prev' => null ], + 'two' => [ 'next' => 'three', 'prev' => 'one' ], + 'three' => [ 'next' => null, 'prev' => 'two' ], + 'zero' => [ 'next' => 'one', 'prev' => 'three'], + 'four' => [ 'next' => null, 'prev' => 'three'] + ]; + // All items are iterated (order not deterministic) + $this->assertEquals( + $expected, + $seen, + 'New items are iterated over' + ); + } + + /** + * Test that items can be modified during iteration + */ + public function testIterateVolatileModified() + { + $initial = [ + 'one' => [ 'next' => 'two', 'prev' => null ], + 'two' => [ 'next' => 'three', 'prev' => 'one' ], + 'three' => [ 'next' => 'four', 'prev' => 'two' ], + 'four' => [ 'next' => null, 'prev' => 'three' ], + ]; + + // Test new items are iterated + $items = $initial; + $seen = []; + foreach (ArrayLib::iterateVolatile($items) as $key => $value) { + $seen[$key] = $value; + // One modifies two + if ($key === 'one') { + $items['two']['modifiedby'] = 'one'; + } + // Two removes three, preventing it from being iterated next + if ($key === 'two') { + unset($items['three']); + } + // Four removes two, but since it's already been iterated by this point + // it's too late. + if ($key === 'four') { + unset($items['two']); + } + } + $expected = [ + 'one' => [ 'next' => 'two', 'prev' => null ], + 'two' => [ 'next' => 'three', 'prev' => 'one', 'modifiedby' => 'one' ], + 'four' => [ 'next' => null, 'prev' => 'three' ], + ]; + // All items are iterated (order not deterministic) + $this->assertEquals( + ksort($expected), + ksort($seen), + 'New items are iterated over' + ); + } } From f952ef747b2e6d86717939edb4008283d71e8a74 Mon Sep 17 00:00:00 2001 From: Colin Tucker Date: Tue, 31 Oct 2017 20:17:55 +1100 Subject: [PATCH 2/4] Fixed array/object mismatch bug in PaginatedList --- src/Control/HTTP.php | 2 +- src/ORM/PaginatedList.php | 27 ++++++++++++++++----------- tests/php/Control/HTTPTest.php | 6 +++--- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/Control/HTTP.php b/src/Control/HTTP.php index 1cff05eb2..cd407bdd7 100644 --- a/src/Control/HTTP.php +++ b/src/Control/HTTP.php @@ -167,7 +167,7 @@ class HTTP * @param string $separator Separator for http_build_query(). * @return string */ - public static function setGetVar($varname, $varvalue, $currentURL = null, $separator = '&') + public static function setGetVar($varname, $varvalue, $currentURL = null, $separator = '&') { if (!isset($currentURL)) { $request = Controller::curr()->getRequest(); diff --git a/src/ORM/PaginatedList.php b/src/ORM/PaginatedList.php index 646075c94..7df431e5a 100644 --- a/src/ORM/PaginatedList.php +++ b/src/ORM/PaginatedList.php @@ -3,6 +3,7 @@ namespace SilverStripe\ORM; use SilverStripe\Control\HTTP; +use SilverStripe\Control\HTTPRequest; use SilverStripe\ORM\Queries\SQLSelect; use SilverStripe\View\ArrayData; use ArrayAccess; @@ -259,7 +260,11 @@ class PaginatedList extends ListDecorator for ($i = $start; $i < $end; $i++) { $result->push(new ArrayData(array( 'PageNum' => $i + 1, - 'Link' => HTTP::setGetVar($this->getPaginationGetVar(), $i * $this->getPageLength()), + 'Link' => HTTP::setGetVar( + $this->getPaginationGetVar(), + $i * $this->getPageLength(), + ($this->request instanceof HTTPRequest) ? $this->request->getURL(true) : null + ), 'CurrentBool' => $this->CurrentPage() == ($i + 1) ))); } @@ -330,7 +335,11 @@ class PaginatedList extends ListDecorator } for ($i = 0; $i < $total; $i++) { - $link = HTTP::setGetVar($this->getPaginationGetVar(), $i * $this->getPageLength()); + $link = HTTP::setGetVar( + $this->getPaginationGetVar(), + $i * $this->getPageLength(), + ($this->request instanceof HTTPRequest) ? $this->request->getURL(true) : null + ); $num = $i + 1; $emptyRange = $num != 1 && $num != $total && ( @@ -439,8 +448,7 @@ class PaginatedList extends ListDecorator return HTTP::setGetVar( $this->getPaginationGetVar(), 0, - $this->request ? $this->request->getURL(true) : null, - '&' + ($this->request instanceof HTTPRequest) ? $this->request->getURL(true) : null ); } @@ -454,8 +462,7 @@ class PaginatedList extends ListDecorator return HTTP::setGetVar( $this->getPaginationGetVar(), ($this->TotalPages() - 1) * $this->getPageLength(), - $this->request ? $this->request->getURL(true) : null, - '&' + ($this->request instanceof HTTPRequest) ? $this->request->getURL(true) : null ); } @@ -471,8 +478,7 @@ class PaginatedList extends ListDecorator return HTTP::setGetVar( $this->getPaginationGetVar(), $this->getPageStart() + $this->getPageLength(), - $this->request ? $this->request->getURL(true) : null, - '&' + ($this->request instanceof HTTPRequest) ? $this->request->getURL(true) : null ); } } @@ -489,8 +495,7 @@ class PaginatedList extends ListDecorator return HTTP::setGetVar( $this->getPaginationGetVar(), $this->getPageStart() - $this->getPageLength(), - $this->request ? $this->request->getURL(true) : null, - '&' + ($this->request instanceof HTTPRequest) ? $this->request->getURL(true) : null ); } } @@ -506,7 +511,7 @@ class PaginatedList extends ListDecorator /** * Set the request object for this list * - * @param HTTPRequest + * @param HTTPRequest|ArrayAccess */ public function setRequest($request) { diff --git a/tests/php/Control/HTTPTest.php b/tests/php/Control/HTTPTest.php index 20cf0eb75..fdc3cec4b 100644 --- a/tests/php/Control/HTTPTest.php +++ b/tests/php/Control/HTTPTest.php @@ -154,13 +154,13 @@ class HTTPTest extends FunctionalTest ); $this->assertEquals( - 'relative/url?baz=buz&foo=bar', + 'relative/url?baz=buz&foo=bar', HTTP::setGetVar('foo', 'bar', '/relative/url?baz=buz'), 'Relative URL with existing query params, and new added key' ); $this->assertEquals( - 'http://test.com/?foo=new&buz=baz', + 'http://test.com/?foo=new&buz=baz', HTTP::setGetVar('foo', 'new', 'http://test.com/?foo=old&buz=baz'), 'Absolute URL without path and multipe existing query params, overwriting an existing parameter' ); @@ -172,7 +172,7 @@ class HTTPTest extends FunctionalTest ); // http_build_query() escapes angular brackets, they should be correctly urldecoded by the browser client $this->assertEquals( - 'http://test.com/?foo%5Btest%5D=one&foo%5Btest%5D=two', + 'http://test.com/?foo%5Btest%5D=one&foo%5Btest%5D=two', HTTP::setGetVar('foo[test]', 'two', 'http://test.com/?foo[test]=one'), 'Absolute URL and PHP array query string notation' ); From 3d4c50d76ce448134fec1885a4ecc0a8caa9249b Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 14 Nov 2017 16:07:23 +1300 Subject: [PATCH 3/4] DOC Improve file permission docs See #7584 --- .../01_Installation/05_Common_Problems.md | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/docs/en/00_Getting_Started/01_Installation/05_Common_Problems.md b/docs/en/00_Getting_Started/01_Installation/05_Common_Problems.md index 5c04499a0..7c5dcbc87 100644 --- a/docs/en/00_Getting_Started/01_Installation/05_Common_Problems.md +++ b/docs/en/00_Getting_Started/01_Installation/05_Common_Problems.md @@ -87,10 +87,21 @@ needs to create/have write-access to: * Image thumbnails will not show in the CMS if permission is not given If you are running on a server instance where users other than the webserver user will need -read / write access to files in the assets folder, then you will need to adjust the -permissions of the filesystem to a more permissive setting. +read / write access to files in the assets folder, then you should do one of the below: -By default private files and `.htaccess` are written with permission `0644`. + - Ensure that all server users that modify this file belong to the same group + - Modify the permissions that SilverStripe uses to write to a more permissive setting (see below) + +It may be necessary to manually set the original permissions, and owner user/group of your assets folder on +initial deployment. + +For more information on understanding and determining file permissions, please see +[wikipedia](https://en.wikipedia.org/wiki/File_system_permissions#Traditional_Unix_permissions) +on unix permissions. + +### Modifying write permissions of files + +By default all files and `.htaccess` are written with permission `0664`. You could enable other users to access these files with the below config. Note: Please adjust the values below to those appropriate for your server configuration. You may require `0666` for combined files generated during requests where they are cleared or refreshed only during a flush. @@ -104,16 +115,13 @@ Name: myassetperms SilverStripe\Assets\Flysystem\AssetAdapter: file_permissions: file: - public: 0644 - private: 0644 + public: 0666 # Replace with your desired permission for files dir: - public: 0755 - private: 0755 + public: 0777 # Replace with your desired permission for folders ``` -For more information on understanding and determining file permissions, please see -[wikipedia](https://en.wikipedia.org/wiki/File_system_permissions#Traditional_Unix_permissions) -on unix permissions. +Note: `public` key applies to all files whether they are protected or public; This is a flag internal to +Flysystem, and file protection is applied by SilverStripe on top of these permissions. ## I have whitespace before my HTML output, triggering quirks mode or preventing cookies from being set From 8b063026f0c393117c95306412bbabf35fa14335 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 15 Nov 2017 13:30:19 +1300 Subject: [PATCH 4/4] ENHANCEMENT Ensure that non-writable assets files are notified during install Fixes #7580 --- src/Dev/Install/InstallRequirements.php | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/Dev/Install/InstallRequirements.php b/src/Dev/Install/InstallRequirements.php index fce65e1cb..736603b95 100644 --- a/src/Dev/Install/InstallRequirements.php +++ b/src/Dev/Install/InstallRequirements.php @@ -5,7 +5,10 @@ namespace SilverStripe\Dev\Install; use BadMethodCallException; use Exception; use InvalidArgumentException; +use RecursiveDirectoryIterator; +use RecursiveIteratorIterator; use SilverStripe\Core\TempFolder; +use SplFileInfo; /** * This class checks requirements @@ -292,8 +295,24 @@ class InstallRequirements null )); } + + + // Ensure root assets dir is writable $this->requireWriteable('assets', array("File permissions", "Is the assets/ directory writeable?", null)); + // Ensure all assets files are writable + $assetsDir = $this->getBaseDir() . 'assets'; + $innerIterator = new RecursiveDirectoryIterator($assetsDir, RecursiveDirectoryIterator::SKIP_DOTS); + $iterator = new RecursiveIteratorIterator($innerIterator, RecursiveIteratorIterator::SELF_FIRST); + /** @var SplFileInfo $file */ + foreach ($iterator as $file) { + $relativePath = substr($file->getPathname(), strlen($this->getBaseDir())); + $message = $file->isDir() + ? "Is the {$relativePath} directory writeable?" + : "Is the {$relativePath} file writeable?"; + $this->requireWriteable($relativePath, array("File permissions", $message, null)); + } + try { $tempFolder = TempFolder::getTempFolder($this->getBaseDir()); } catch (Exception $e) {