Merge remote-tracking branch 'origin/4.0' into 4

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
 

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();

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) {

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.',

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];
+                }
+            }
+        }
+    }
 }

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

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)
     {

src/View/Parsers/Diff.php

@@ -181,16 +181,15 @@ class Diff extends \Diff
         $content = str_replace(array("&nbsp;", "<", ">"), 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;

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'
         );

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'
+        );
+    }
 }