From d3a4161a945812cecdf83362cde8fcb9dbfd24cd Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 5 Jun 2013 14:59:01 +0200 Subject: [PATCH 01/54] Behat: Backport improved dropdown step handler --- .../Framework/Test/Behaviour/CmsUiContext.php | 98 ++++++++++++------- 1 file changed, 65 insertions(+), 33 deletions(-) diff --git a/tests/behat/features/bootstrap/SilverStripe/Framework/Test/Behaviour/CmsUiContext.php b/tests/behat/features/bootstrap/SilverStripe/Framework/Test/Behaviour/CmsUiContext.php index 9d263dcc9..f6078ff17 100644 --- a/tests/behat/features/bootstrap/SilverStripe/Framework/Test/Behaviour/CmsUiContext.php +++ b/tests/behat/features/bootstrap/SilverStripe/Framework/Test/Behaviour/CmsUiContext.php @@ -273,55 +273,87 @@ class CmsUiContext extends BehatContext { $field = $this->fixStepArgument($field); $value = $this->fixStepArgument($value); + $nativeField = $this->getSession()->getPage()->findField($field); if($nativeField) { $nativeField->selectOption($value); - } else { - // TODO Allow searching by label - $inputField = $this->getSession()->getPage()->find('xpath', "//*[@name='$field']"); - if(null === $inputField) { - throw new \InvalidArgumentException(sprintf( - 'Chosen.js dropdown named "%s" not found', - $field - )); - } + return; + } + // Given the fuzzy matching, we might get more than one matching field. + $formFields = array(); + + // Find by label + $formField = $this->getSession()->getPage()->findField($field); + if($formField) $formFields[] = $formField; + + // Fall back to finding by title (for dropdowns without a label) + if(!$formFields) { + $formFields = $this->getSession()->getPage()->findAll( + 'xpath', + sprintf( + '//*[self::select][(./@title="%s")]', + $field + ) + ); + } + + // Find by name (incl. hidden fields) + if(!$formFields) { + $formFields = $this->getSession()->getPage()->findAll('xpath', "//*[@name='$field']"); + } + + assertGreaterThan(0, count($formFields), sprintf( + 'Chosen.js dropdown named "%s" not found', + $field + )); + + // Traverse up to field holder + $containers = array(); + foreach($formFields as $formField) { do { - $container = $inputField->getParent(); + $container = $formField->getParent(); $containerClasses = explode(' ', $container->getAttribute('class')); + $containers[] = $container; } while( $container && in_array('field', $containerClasses) && $container->getTagName() != 'form' ); - if(null === $container) throw new \InvalidArgumentException('Chosen.js field container not found'); + } + + assertGreaterThan(0, count($containers), 'Chosen.js field container not found'); - $triggerEl = $container->find('xpath', './/a'); - if(null === $triggerEl) throw new \InvalidArgumentException('Chosen.js link element not found'); - $triggerEl->click(); + // Default to first visible container + $container = $containers[0]; + + // Click on newly expanded list element, indirectly setting the dropdown value + $linkEl = $container->find('xpath', './/a[./@href]'); + assertNotNull($linkEl, 'Chosen.js link element not found'); + $this->getSession()->wait(100); // wait for dropdown overlay to appear + $linkEl->click(); - if(in_array('treedropdown', $containerClasses)) { - // wait for ajax dropdown to load - $this->getSession()->wait( - 5000, - "jQuery('#" . $container->getAttribute('id') . " .treedropdownfield-panel li').length > 0" - ); - } else { - // wait for dropdown overlay to appear - $this->getSession()->wait(100); - } - - $listEl = $container->find('xpath', sprintf('.//li[contains(normalize-space(string(.)), \'%s\')]', $value)); - if(null === $listEl) { - throw new \InvalidArgumentException(sprintf( - 'Chosen.js list element with title "%s" not found', - $value - )); - } - $listEl->find('xpath', './/a')->click(); + if(in_array('treedropdown', $containerClasses)) { + // wait for ajax dropdown to load + $this->getSession()->wait( + 5000, + "jQuery('#" . $container->getAttribute('id') . " .treedropdownfield-panel li').length > 0" + ); + } else { + // wait for dropdown overlay to appear (might be animated) + $this->getSession()->wait(300); } + $listEl = $container->find('xpath', sprintf('.//li[contains(normalize-space(string(.)), \'%s\')]', $value)); + if(null === $listEl) { + throw new \InvalidArgumentException(sprintf( + 'Chosen.js list element with title "%s" not found', + $value + )); + } + + $listEl->find('xpath', './/a')->click(); } /** From 6aae3d7d0581d50a09b21fc07186eaa74c7b21c0 Mon Sep 17 00:00:00 2001 From: Stevie Mayhew Date: Fri, 7 Jun 2013 12:33:57 +1200 Subject: [PATCH 02/54] MINOR: equality check consistency Updated all equality logic checks to use double == for consistency across the page. --- docs/en/reference/templates.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/en/reference/templates.md b/docs/en/reference/templates.md index 67356261b..d5f2d3959 100644 --- a/docs/en/reference/templates.md +++ b/docs/en/reference/templates.md @@ -146,7 +146,7 @@ quotes, `kipper`, which is a **literal**. If true, the text inside the if-block is output. :::ss - <% if $MyDinner="kipper" %> + <% if $MyDinner=="kipper" %> Yummy, kipper for tea. <% end_if %> @@ -159,7 +159,7 @@ This example shows the use of the `else` option. The markup after `else` is output if the tested condition is *not* true. :::ss - <% if $MyDinner="kipper" %> + <% if $MyDinner=="kipper" %> Yummy, kipper for tea <% else %> I wish I could have kipper :-( @@ -171,9 +171,9 @@ and the markup for that condition is used. If none of the conditions are true, the markup in the `else` clause is used, if that clause is present. :::ss - <% if $MyDinner="quiche" %> + <% if $MyDinner=="quiche" %> Real men don't eat quiche - <% else_if $MyDinner=$YourDinner %> + <% else_if $MyDinner==$YourDinner %> We both have good taste <% else %> Can I have some of your chips? From 71a5615213e31f04c10e82fd6e69453ccdbb5887 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Mon, 10 Jun 2013 11:51:35 +0200 Subject: [PATCH 03/54] Test $allowed_actions on controllers with template name=action conventions --- tests/control/ControllerTest.php | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/control/ControllerTest.php b/tests/control/ControllerTest.php index a47bbba5a..be5c61ee0 100644 --- a/tests/control/ControllerTest.php +++ b/tests/control/ControllerTest.php @@ -55,6 +55,20 @@ class ControllerTest extends FunctionalTest { 'even if action is unsecured on parent class' ); + $response = $this->get("ControllerTest_AccessSecuredController/templateaction"); + $this->assertEquals(403, $response->getStatusCode(), + 'Access denied on action with $allowed_actions on defining controller, ' . + 'if action is not a method but rather a template discovered by naming convention' + ); + + $this->session()->inst_set('loggedInAs', $adminUser->ID); + $response = $this->get("ControllerTest_AccessSecuredController/templateaction"); + $this->assertEquals(200, $response->getStatusCode(), + 'Access granted for logged in admin on action with $allowed_actions on defining controller, ' . + 'if action is not a method but rather a template discovered by naming convention' + ); + $this->session()->inst_set('loggedInAs', null); + $response = $this->get("ControllerTest_AccessSecuredController/adminonly"); $this->assertEquals(403, $response->getStatusCode(), 'Access denied on action with $allowed_actions on defining controller, ' . @@ -296,6 +310,12 @@ class ControllerTest_AccessSecuredController extends ControllerTest_AccessBaseCo static $allowed_actions = array( "onlysecuredinsubclassaction" => 'ADMIN', "adminonly" => "ADMIN", + // Defined as ControllerTest_templateaction + 'templateaction' => 'ADMIN' + ); + + protected $templates = array( + 'templateaction' => 'ControllerTest_templateaction' ); // Accessible by ADMIN only @@ -315,6 +335,7 @@ class ControllerTest_AccessSecuredController extends ControllerTest_AccessBaseCo public function adminonly() { return "You must be an admin!"; } + } /** From 3b40711b98970a7e0f0b6709fbdc6d3ddd55dea0 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Thu, 13 Jun 2013 17:31:28 +0200 Subject: [PATCH 04/54] BUG Resize infinite loops in IE8 (fixes #575) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IE8 gets a bit confused and fires resize events when element dimensions in the DOM are changed as a result of a resize event, causing an infinite loop. Apart from artificially throttling the event, the only solution I've found is to check for actual window dimension changes. http://stackoverflow.com/questions/12366315/window-resize-event-continually-fires-in-ie7?lq=1 This implicitly fixes an issue where TreeDropdownField panel isn't accessible in the "Insert Media" popup, because the resize event happes to be triggered by the popup overlay, and in effect closes the drop down panel right after opening it. Relating to the jQuery UI component, there's a host of issues and discussions around this, but no solution… http://bugs.jquery.com/ticket/4097 http://bugs.jqueryui.com/ticket/4758 http://bugs.jqueryui.com/ticket/4065 http://bugs.jqueryui.com/ticket/7514 http://bugs.jqueryui.com/ticket/8881 https://groups.google.com/forum/?fromgroups#!topic/jquery-ui/fDSvwAKL6Go http://www.mail-archive.com/jquery-ui@googlegroups.com/msg04839.html --- admin/javascript/LeftAndMain.js | 24 +++++++++++++++++++++++- javascript/TreeDropdownField.js | 18 ++++++++++++++++-- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/admin/javascript/LeftAndMain.js b/admin/javascript/LeftAndMain.js index 09a948267..66a3445f9 100644 --- a/admin/javascript/LeftAndMain.js +++ b/admin/javascript/LeftAndMain.js @@ -4,6 +4,25 @@ jQuery.noConflict(); * File: LeftAndMain.js */ (function($) { + + var windowWidth, windowHeight; + $(window).bind('resize.leftandmain', function(e) { + // Entwine's 'fromWindow::onresize' does not trigger on IE8. Use synthetic event. + var cb = function() {$('.cms-container').trigger('windowresize');}; + + // Workaround to avoid IE8 infinite loops when elements are resized as a result of this event + if($.browser.msie && parseInt($.browser.version, 10) < 9) { + var newWindowWidth = $(window).width(), newWindowHeight = $(window).height(); + if(newWindowWidth != windowWidth || newWindowHeight != windowHeight) { + windowWidth = newWindowWidth; + windowHeight = newWindowHeight; + cb(); + } + } else { + cb(); + } + }); + // setup jquery.entwine $.entwine.warningLevel = $.entwine.WARN_LEVEL_BESTPRACTISE; $.entwine('ss', function($) { @@ -133,7 +152,10 @@ jQuery.noConflict(); fromWindow: { onstatechange: function(){ this.handleStateChange(); }, - onresize: function(){ this.redraw(); } + }, + + 'onwindowresize': function() { + this.redraw(); }, 'from .cms-panel': { diff --git a/javascript/TreeDropdownField.js b/javascript/TreeDropdownField.js index 45b5afb5c..659ffe291 100644 --- a/javascript/TreeDropdownField.js +++ b/javascript/TreeDropdownField.js @@ -4,8 +4,22 @@ * On resize of any close the open treedropdownfields * as we'll need to redo with widths */ - $(window).resize(function() { - $('.TreeDropdownField').closePanel(); + var windowWidth, windowHeight; + $(window).bind('resize.treedropdownfield', function() { + // Entwine's 'fromWindow::onresize' does not trigger on IE8. Use synthetic event. + var cb = function() {$('.TreeDropdownField').closePanel();}; + + // Workaround to avoid IE8 infinite loops when elements are resized as a result of this event + if($.browser.msie && parseInt($.browser.version, 10) < 9) { + var newWindowWidth = $(window).width(), newWindowHeight = $(window).height(); + if(newWindowWidth != windowWidth || newWindowHeight != windowHeight) { + windowWidth = newWindowWidth; + windowHeight = newWindowHeight; + cb(); + } + } else { + cb(); + } }); var strings = { From 964b3f2d48e15348c9ae3891017b8428019a10d3 Mon Sep 17 00:00:00 2001 From: Jeremy Thomerson Date: Sat, 15 Jun 2013 13:40:33 +0000 Subject: [PATCH 05/54] FIX: <% if Link %> wasn't working Since ViewableData was returning a casting helper for Link, but DataObject was only using $this->$fieldname to set values on that casting helper, you could not use <% if Link %> (or <% if $Link %>) in your templates because Link is not a field, and thus had no value to be set on the casting helper, causing hasValue to think that there was no value. Since DataObject->dbObject says that "it only matches fields and not methods", it seems safe to have it call db(..) to get the field spec, and not call ViewableData->castingHelper at all. --- model/DataObject.php | 4 ++-- tests/view/SSViewerTest.php | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/model/DataObject.php b/model/DataObject.php index 70749387f..0b6109860 100644 --- a/model/DataObject.php +++ b/model/DataObject.php @@ -2643,8 +2643,8 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity } else if($fieldName == 'ID') { return new PrimaryKey($fieldName, $this); - // General casting information for items in $db or $casting - } else if($helper = $this->castingHelper($fieldName)) { + // General casting information for items in $db + } else if($helper = $this->db($fieldName)) { $obj = Object::create_from_string($helper, $fieldName); $obj->setValue($this->$fieldName, $this->record, false); return $obj; diff --git a/tests/view/SSViewerTest.php b/tests/view/SSViewerTest.php index 92b181b28..15003e9d9 100644 --- a/tests/view/SSViewerTest.php +++ b/tests/view/SSViewerTest.php @@ -165,6 +165,18 @@ SS; 'Permissions template functions result correct result'); } + public function testNonFieldCastingHelpersNotUsedInHasValue() { + // check if Link without $ in front of variable + $result = $this->render( + 'A<% if Link %>$Link<% end_if %>B', new SSViewerTest_Object()); + $this->assertEquals('Asome/url.htmlB', $result, 'casting helper not used for <% if Link %>'); + + // check if Link with $ in front of variable + $result = $this->render( + 'A<% if $Link %>$Link<% end_if %>B', new SSViewerTest_Object()); + $this->assertEquals('Asome/url.htmlB', $result, 'casting helper not used for <% if $Link %>'); + } + public function testLocalFunctionsTakePriorityOverGlobals() { $data = new ArrayData(array( 'Page' => new SSViewerTest_Object() @@ -1274,6 +1286,11 @@ class SSViewerTest_Object extends DataObject { public $number = null; + private static $casting = array( + 'Link' => 'Text', + ); + + public function __construct($number = null) { parent::__construct(); $this->number = $number; @@ -1290,6 +1307,10 @@ class SSViewerTest_Object extends DataObject { public function lotsOfArguments11($a, $b, $c, $d, $e, $f, $g, $h, $i, $j, $k) { return $a. $b. $c. $d. $e. $f. $g. $h. $i. $j. $k; } + + public function Link() { + return 'some/url.html'; + } } class SSViewerTest_GlobalProvider implements TemplateGlobalProvider, TestOnly { From 671b7a0cc7925fc855ec656f4be36edb6429f915 Mon Sep 17 00:00:00 2001 From: CheeseSucker Date: Tue, 18 Jun 2013 15:50:32 +0300 Subject: [PATCH 06/54] Consolidated command line examples Examples were broken into several
 blocks.
---
 docs/en/topics/testing/index.md | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/docs/en/topics/testing/index.md b/docs/en/topics/testing/index.md
index ec29bb2ca..9f2053d4b 100644
--- a/docs/en/topics/testing/index.md
+++ b/docs/en/topics/testing/index.md
@@ -56,16 +56,16 @@ The `phpunit` binary should be used from the root directory of your website.
 
 	# Runs all tests defined in phpunit.xml
 	phpunit
-
+	
 	# Run all tests of a specific module
  	phpunit framework/tests/
-
+	
  	# Run specific tests within a specific module
  	phpunit framework/tests/filesystem
-
+	
  	# Run a specific test
 	phpunit framework/tests/filesystem/FolderTest.php
-
+	
 	# Run tests with optional `$_GET` parameters (you need an empty second argument)
  	phpunit framework/tests '' flush=all
 
@@ -81,16 +81,16 @@ particularly around formatting test output.
 
 	# Run all tests
 	sake dev/tests/all
-
+	
 	# Run all tests of a specific module (comma-separated)
 	sake dev/tests/module/framework,cms
-
+	
 	# Run specific tests (comma-separated)
 	sake dev/tests/FolderTest,OtherTest
-
+	
 	# Run tests with optional `$_GET` parameters
 	sake dev/tests/all flush=all
-
+	
 	# Skip some tests
 	sake dev/tests/all SkipTests=MySkippedTest
 
@@ -187,4 +187,4 @@ understand the problem space and discover suitable APIs for performing specific
 **Behavior Driven Development (BDD):** An extension of the test-driven programming style, where tests are used primarily
 for describing the specification of how code should perform. In practice, there's little or no technical difference - it
 all comes down to language. In BDD, the usual terminology is changed to reflect this change of focus, so *Specification*
-is used in place of *Test Case*, and *should* is used in place of *expect* and *assert*.
\ No newline at end of file
+is used in place of *Test Case*, and *should* is used in place of *expect* and *assert*.

From 36d9563da8011c93ab00328c6392186c06b279de Mon Sep 17 00:00:00 2001
From: Ryan O'Hara 
Date: Wed, 19 Jun 2013 11:27:22 +1200
Subject: [PATCH 07/54] FIX make sure select dropdowns in add page dialog
 aren't cut off due to .parent-mode class having overflow:auto

---
 admin/css/screen.css   | 9 +++++----
 admin/scss/_style.scss | 2 ++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/admin/css/screen.css b/admin/css/screen.css
index 3d4aeaba0..b9eba59ed 100644
--- a/admin/css/screen.css
+++ b/admin/css/screen.css
@@ -460,7 +460,8 @@ body.cms { overflow: hidden; }
 .cms-add-form .step-label { opacity: 0.9; }
 .cms-add-form .step-label .flyout { height: 17px; padding-top: 5px; }
 .cms-add-form .step-label .title { padding-top: 5px; font-weight: bold; text-shadow: 1px 1px 0 white; }
-.cms-add-form ul.SelectionGroup { padding-left: 28px; }
+.cms-add-form ul.SelectionGroup { padding-left: 28px; overflow: visible; *zoom: 1; }
+.cms-add-form ul.SelectionGroup:after { content: "\0020"; display: block; height: 0; clear: both; overflow: hidden; visibility: hidden; }
 .cms-add-form .parent-mode { padding: 8px; overflow: auto; }
 
 #PageType ul { padding-left: 20px; }
@@ -748,7 +749,7 @@ form.import-form label.left { width: 250px; }
 .cms .jstree .jstree-wholerow-real, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow-real { position: relative; z-index: 1; }
 .cms .jstree .jstree-wholerow-real li, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow-real li { cursor: pointer; }
 .cms .jstree .jstree-wholerow-real a, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow-real a { border-left-color: transparent !important; border-right-color: transparent !important; }
-.cms .jstree .jstree-wholerow, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow, .cms .jstree .jstree-wholerow ul, .cms .jstree .jstree-wholerow li, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow ul, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow ul, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow li, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow li, .cms .jstree .jstree-wholerow a, .cms .jstree .jstree-wholerow a:hover, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow a, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow a, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow a:hover, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow a:hover, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow ul, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow ul, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow li, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow li, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow ul, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow li, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow a, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow a, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow a:hover, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow a:hover, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow a, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow a:hover { margin: 0 !important; padding: 0 !important; }
+.cms .jstree .jstree-wholerow, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow, .cms .jstree .jstree-wholerow ul, .cms .jstree .jstree-wholerow li, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow ul, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow ul, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow li, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow li, .cms .jstree .jstree-wholerow a, .cms .jstree .jstree-wholerow a:hover, .cms .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow a, .TreeDropdownField .treedropdownfield-panel .cms .jstree .jstree-wholerow a, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow ul, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow li, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow a, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow a:hover { margin: 0 !important; padding: 0 !important; }
 .cms .jstree .jstree-wholerow, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow { position: relative; z-index: 0; height: 0; background: transparent !important; }
 .cms .jstree .jstree-wholerow ul, .cms .jstree .jstree-wholerow li, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow ul, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow li { background: transparent !important; width: 100%; }
 .cms .jstree .jstree-wholerow a, .cms .jstree .jstree-wholerow a:hover, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow a, .TreeDropdownField .treedropdownfield-panel .jstree .jstree-wholerow a:hover { text-indent: -9999px !important; width: 100%; border-right-width: 0px !important; border-left-width: 0px !important; }
@@ -767,7 +768,7 @@ form.import-form label.left { width: 250px; }
 .cms .jstree-themeroller a, .TreeDropdownField .treedropdownfield-panel .jstree-themeroller a { padding: 0 2px; }
 .cms .jstree-themeroller .ui-icon, .TreeDropdownField .treedropdownfield-panel .jstree-themeroller .ui-icon { overflow: visible; }
 .cms .jstree-themeroller .jstree-no-icon, .TreeDropdownField .treedropdownfield-panel .jstree-themeroller .jstree-no-icon { display: none; }
-.cms #jstree-marker, .cms .TreeDropdownField .treedropdownfield-panel #jstree-marker, .TreeDropdownField .treedropdownfield-panel .cms #jstree-marker, .cms #jstree-marker-line, .cms .TreeDropdownField .treedropdownfield-panel #jstree-marker-line, .TreeDropdownField .treedropdownfield-panel .cms #jstree-marker-line, .TreeDropdownField .treedropdownfield-panel .cms #jstree-marker, .cms .TreeDropdownField .treedropdownfield-panel #jstree-marker, .TreeDropdownField .treedropdownfield-panel #jstree-marker, .TreeDropdownField .treedropdownfield-panel .cms #jstree-marker-line, .cms .TreeDropdownField .treedropdownfield-panel #jstree-marker-line, .TreeDropdownField .treedropdownfield-panel #jstree-marker-line { padding: 0; margin: 0; overflow: hidden; position: absolute; top: -30px; background-repeat: no-repeat; display: none; }
+.cms #jstree-marker, .cms .TreeDropdownField .treedropdownfield-panel #jstree-marker, .TreeDropdownField .treedropdownfield-panel .cms #jstree-marker, .cms #jstree-marker-line, .cms .TreeDropdownField .treedropdownfield-panel #jstree-marker-line, .TreeDropdownField .treedropdownfield-panel .cms #jstree-marker-line, .TreeDropdownField .treedropdownfield-panel #jstree-marker, .TreeDropdownField .treedropdownfield-panel #jstree-marker-line { padding: 0; margin: 0; overflow: hidden; position: absolute; top: -30px; background-repeat: no-repeat; display: none; }
 .cms #jstree-marker, .TreeDropdownField .treedropdownfield-panel #jstree-marker { line-height: 10px; font-size: 12px; height: 12px; width: 8px; z-index: 10001; background-color: transparent; text-shadow: 1px 1px 1px white; color: black; }
 .cms #jstree-marker-line, .TreeDropdownField .treedropdownfield-panel #jstree-marker-line { line-height: 0%; font-size: 1px; height: 1px; width: 100px; z-index: 10000; background-color: #456c43; cursor: pointer; border: 1px solid #eeeeee; border-left: 0; -moz-box-shadow: 0px 0px 2px #666; -webkit-box-shadow: 0px 0px 2px #666; box-shadow: 0px 0px 2px #666; -moz-border-radius: 1px; border-radius: 1px; -webkit-border-radius: 1px; }
 .cms #vakata-contextmenu, .TreeDropdownField .treedropdownfield-panel #vakata-contextmenu { display: block; visibility: hidden; left: 0; top: -200px; position: absolute; margin: 0; padding: 0; min-width: 180px; background: #FFF; border: 1px solid silver; z-index: 10000; *width: 180px; -webkit-box-shadow: 0 0 10px #cccccc; -moz-box-shadow: 0 0 10px #cccccc; box-shadow: 0 0 10px #cccccc; }
@@ -810,7 +811,7 @@ form.import-form label.left { width: 250px; }
 .tree-holder.jstree-apple span.badge.status-deletedonlive, .tree-holder.jstree-apple span.badge.status-removedfromdraft, .cms-tree.jstree-apple span.badge.status-deletedonlive, .cms-tree.jstree-apple span.badge.status-removedfromdraft { color: #636363; border: 1px solid #E49393; background-color: #F2DADB; }
 .tree-holder.jstree-apple span.badge.status-workflow-approval, .cms-tree.jstree-apple span.badge.status-workflow-approval { color: #56660C; border: 1px solid #7C8816; background-color: #DAE79A; }
 .tree-holder.jstree-apple span.comment-count, .cms-tree.jstree-apple span.comment-count { clear: both; position: relative; text-transform: uppercase; display: inline-block; overflow: visible; padding: 0px 3px; font-size: 0.75em; line-height: 1em; margin-left: 3px; margin-right: 6px; -webkit-border-radius: 2px 2px; -moz-border-radius: 2px / 2px; border-radius: 2px / 2px; color: #7E7470; border: 1px solid #C9B800; background-color: #FFF0BC; }
-.tree-holder.jstree-apple span.comment-count span.comment-count:before, .tree-holder.jstree-apple span.comment-count .cms-tree.jstree-apple span.comment-count:before, .cms-tree.jstree-apple .tree-holder.jstree-apple span.comment-count span.comment-count:before, .tree-holder.jstree-apple span.comment-count span.comment-count:after, .tree-holder.jstree-apple span.comment-count .cms-tree.jstree-apple span.comment-count:after, .cms-tree.jstree-apple .tree-holder.jstree-apple span.comment-count span.comment-count:after, .cms-tree.jstree-apple span.comment-count .tree-holder.jstree-apple span.comment-count:before, .tree-holder.jstree-apple .cms-tree.jstree-apple span.comment-count span.comment-count:before, .cms-tree.jstree-apple span.comment-count span.comment-count:before, .cms-tree.jstree-apple span.comment-count .tree-holder.jstree-apple span.comment-count:after, .tree-holder.jstree-apple .cms-tree.jstree-apple span.comment-count span.comment-count:after, .cms-tree.jstree-apple span.comment-count span.comment-count:after { content: ""; position: absolute; border-style: solid; /* reduce the damage in FF3.0 */ display: block; width: 0; }
+.tree-holder.jstree-apple span.comment-count span.comment-count:before, .tree-holder.jstree-apple span.comment-count span.comment-count:after, .cms-tree.jstree-apple span.comment-count span.comment-count:before, .cms-tree.jstree-apple span.comment-count span.comment-count:after { content: ""; position: absolute; border-style: solid; /* reduce the damage in FF3.0 */ display: block; width: 0; }
 .tree-holder.jstree-apple span.comment-count:before, .cms-tree.jstree-apple span.comment-count:before { bottom: -4px; /* value = - border-top-width - border-bottom-width */ left: 3px; /* controls horizontal position */ border-width: 4px 4px 0; border-color: #C9B800 transparent; }
 .tree-holder.jstree-apple span.comment-count:after, .cms-tree.jstree-apple span.comment-count:after { bottom: -3px; /* value = - border-top-width - border-bottom-width */ left: 4px; /* value = (:before left) + (:before border-left) - (:after border-left) */ border-width: 3px 3px 0; border-color: #FFF0BC transparent; }
 .tree-holder.jstree-apple .jstree-hovered, .cms-tree.jstree-apple .jstree-hovered { text-shadow: none; text-decoration: none; }
diff --git a/admin/scss/_style.scss b/admin/scss/_style.scss
index 5d1548d81..790d1944d 100644
--- a/admin/scss/_style.scss
+++ b/admin/scss/_style.scss
@@ -529,6 +529,8 @@ body.cms {
 	}
 	ul.SelectionGroup {
 		padding-left:28px;
+		overflow: visible;
+		@include legacy-pie-clearfix;
 	}
 	.parent-mode {
 		padding: $grid-x;

From 7a3fcfc83984b77ce7477581fa423904ebce3a1e Mon Sep 17 00:00:00 2001
From: Ryan Wachtl 
Date: Wed, 19 Jun 2013 00:52:12 -0500
Subject: [PATCH 08/54] Missing directory separators in output of suggested
 _ss_environment paths

---
 cli-script.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cli-script.php b/cli-script.php
index 8f3f37727..2b02a693c 100755
--- a/cli-script.php
+++ b/cli-script.php
@@ -75,7 +75,7 @@ require_once("model/DB.php");
 if(!isset($databaseConfig) || !isset($databaseConfig['database']) || !$databaseConfig['database']) {
 	echo "\nPlease configure your database connection details.  You can do this by creating a file
 called _ss_environment.php in either of the following locations:\n\n";
-	echo " - " .  BASE_PATH  ."_ss_environment.php\n - " . dirname(BASE_PATH) . "_ss_environment.php\n\n";
+	echo " - " .  BASE_PATH  ."/_ss_environment.php\n - " . dirname(BASE_PATH) . "/_ss_environment.php\n\n";
 	echo <<
Date: Wed, 19 Jun 2013 13:42:28 +0200
Subject: [PATCH 09/54] Environment Config: SS_DATABASE_MEMORY

---
 conf/ConfigureFromEnv.php | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/conf/ConfigureFromEnv.php b/conf/ConfigureFromEnv.php
index b081ca694..6d2e7dc28 100644
--- a/conf/ConfigureFromEnv.php
+++ b/conf/ConfigureFromEnv.php
@@ -16,6 +16,8 @@
  *  - SS_DATABASE_SUFFIX:   A suffix to add to the database name.
  *  - SS_DATABASE_PREFIX:   A prefix to add to the database name.
  *  - SS_DATABASE_TIMEZONE: Set the database timezone to something other than the system timezone.
+ *  - SS_DATABASE_MEMORY:   Use in-memory state if possible. Useful for testing, currently only  
+ *                          supported by the SQLite database adapter.
  * 
  * There is one more setting that is intended to be used by people who work on SilverStripe.
  *  - SS_DATABASE_CHOOSE_NAME: Boolean/Int.  If set, then the system will choose a default database name for you if
@@ -104,6 +106,10 @@ if(defined('SS_DATABASE_USERNAME') && defined('SS_DATABASE_PASSWORD')) {
 	// For schema enabled drivers: 
 	if(defined('SS_DATABASE_SCHEMA')) 
 		$databaseConfig["schema"] = SS_DATABASE_SCHEMA; 
+
+	// For SQlite3 memory databases (mainly for testing purposes)
+	if(defined('SS_DATABASE_MEMORY')) 
+		$databaseConfig["memory"] = SS_DATABASE_MEMORY; 
 }
 
 if(defined('SS_SEND_ALL_EMAILS_TO')) {

From 8769da5622810983f3c70b16469709a42a4b288b Mon Sep 17 00:00:00 2001
From: Ingo Schommer 
Date: Wed, 19 Jun 2013 14:28:34 +0200
Subject: [PATCH 10/54] CMS UI: Resize iframe alongside dialog

Fixes regression from 9f600ada2cbefd38fb51b9b274827744a64fd4c7
---
 admin/javascript/ssui.core.js | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/admin/javascript/ssui.core.js b/admin/javascript/ssui.core.js
index 74efe5095..dff784fe7 100644
--- a/admin/javascript/ssui.core.js
+++ b/admin/javascript/ssui.core.js
@@ -186,7 +186,7 @@
 			$(window).unbind('resize.ssdialog');
 		},
 		_resizeIframe: function() {
-			var opts = {}, newWidth, newHeight;
+			var opts = {}, newWidth, newHeight, iframe = this.element.children('iframe');;
 			if(this.options.widthRatio) {
 				newWidth = $(window).width() * this.options.widthRatio;
 				if(this.options.minWidth && newWidth < this.options.minWidth) {
@@ -210,8 +210,22 @@
 			if(this.options.autoPosition) {
 				opts.position = this.options.position;
 			}
+
 			if(!jQuery.isEmptyObject(opts)) {
+				// Resize dialog
 				this._setOptions(opts);
+
+				// Resize iframe within dialog
+				iframe.attr('width', 
+					opts.width 
+					- parseFloat(this.element.css('paddingLeft'))
+					- parseFloat(this.element.css('paddingRight'))
+				);
+				iframe.attr('height', 
+					opts.height
+					- parseFloat(this.element.css('paddingTop')) 
+					- parseFloat(this.element.css('paddingBottom'))
+				);
 			}
 		}
 	});

From 2ac344467537d6c5f5ac26c497e7d7dfdc58e803 Mon Sep 17 00:00:00 2001
From: CheeseSucker 
Date: Wed, 19 Jun 2013 16:48:49 +0200
Subject: [PATCH 11/54] MINOR: Fixed typo

---
 docs/en/reference/grid-field.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/en/reference/grid-field.md b/docs/en/reference/grid-field.md
index a3c0ac114..9317f3fec 100644
--- a/docs/en/reference/grid-field.md
+++ b/docs/en/reference/grid-field.md
@@ -1,7 +1,7 @@
 # Gridfield
 
 Gridfield is SilverStripe's implementation of data grids. Its main purpose is to display tabular data
-in a format that is easy to view and modify. It's a can be thought of as a HTML table with some tricks.
+in a format that is easy to view and modify. It can be thought of as a HTML table with some tricks.
 
 It's built in a way that provides developers with an extensible way to display tabular data in a 
 table and minimise the amount of code that needs to be written.

From 0cf6b78b0e3f39dbb5f7cdbd667a264fee7773db Mon Sep 17 00:00:00 2001
From: Ryan Wachtl 
Date: Wed, 19 Jun 2013 10:12:42 -0500
Subject: [PATCH 12/54] Update cli-script.php

Use DIRECTORY_SEPARATOR for cross platform
---
 cli-script.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cli-script.php b/cli-script.php
index 2b02a693c..2978868fe 100755
--- a/cli-script.php
+++ b/cli-script.php
@@ -75,7 +75,7 @@ require_once("model/DB.php");
 if(!isset($databaseConfig) || !isset($databaseConfig['database']) || !$databaseConfig['database']) {
 	echo "\nPlease configure your database connection details.  You can do this by creating a file
 called _ss_environment.php in either of the following locations:\n\n";
-	echo " - " .  BASE_PATH  ."/_ss_environment.php\n - " . dirname(BASE_PATH) . "/_ss_environment.php\n\n";
+	echo " - " .  BASE_PATH  . DIRECTORY_SEPARATOR . "_ss_environment.php\n - " . dirname(BASE_PATH) . DIRECTORY_SEPARATOR . "_ss_environment.php\n\n";
 	echo <<
Date: Thu, 20 Jun 2013 12:30:48 +1200
Subject: [PATCH 13/54] BUG: Fix for Cookie expiry timeout being passed as a
 large number on 64 bit machines

---
 control/Session.php | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/control/Session.php b/control/Session.php
index 43ae43f29..20a9c3905 100644
--- a/control/Session.php
+++ b/control/Session.php
@@ -523,9 +523,11 @@ class Session {
 
 		if(!session_id() && !headers_sent()) {
 			if($domain) {
-				session_set_cookie_params($timeout, $path, $domain, $secure /* secure */, true /* httponly */);
+				session_set_cookie_params($timeout, $path, $domain,
+					$secure /* secure */, true /* httponly */);
 			} else {
-				session_set_cookie_params($timeout, $path, null, $secure /* secure */, true /* httponly */);
+				session_set_cookie_params($timeout, $path, null,
+					$secure /* secure */, true /* httponly */);
 			}
 
 			// Allow storing the session in a non standard location
@@ -541,7 +543,8 @@ class Session {
 		// Modify the timeout behaviour so it's the *inactive* time before the session expires.
 		// By default it's the total session lifetime
 		if($timeout && !headers_sent()) {
-			Cookie::set(session_name(), session_id(), time()+$timeout, $path, $domain ? $domain : null, $secure, true);
+			Cookie::set(session_name(), session_id(), $timeout/86400, $path, $domain ? $domain
+				: null, $secure, true);
 		}
 	}
 

From 328467f1b571f0d9e03bd27222e06e3da5d5042e Mon Sep 17 00:00:00 2001
From: Hamish Friedlander 
Date: Thu, 20 Jun 2013 14:08:46 +1200
Subject: [PATCH 14/54] FIX: ConfirmedPasswordField used to expose existing
 hash

---
 forms/ConfirmedPasswordField.php           |  5 ++++-
 tests/forms/ConfirmedPasswordFieldTest.php | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/forms/ConfirmedPasswordField.php b/forms/ConfirmedPasswordField.php
index 58e2681a4..3f683e5bb 100644
--- a/forms/ConfirmedPasswordField.php
+++ b/forms/ConfirmedPasswordField.php
@@ -239,7 +239,10 @@ class ConfirmedPasswordField extends FormField {
 	 *
 	 * @return ConfirmedPasswordField
 	 */
-	public function setValue($value) {
+	public function setValue($value, $data = null) {
+		// If $data is a DataObject, don't use the value, since it's a hashed value
+		if ($data && $data instanceof DataObject) $value = '';
+
 		if(is_array($value)) {
 			if($value['_Password'] || (!$value['_Password'] && !$this->canBeEmpty)) {
 				$this->value = $value['_Password'];
diff --git a/tests/forms/ConfirmedPasswordFieldTest.php b/tests/forms/ConfirmedPasswordFieldTest.php
index 4f7bfc213..abe8a9794 100644
--- a/tests/forms/ConfirmedPasswordFieldTest.php
+++ b/tests/forms/ConfirmedPasswordFieldTest.php
@@ -15,6 +15,26 @@ class ConfirmedPasswordFieldTest extends SapphireTest {
 		$this->assertEquals('valueB', $field->children->fieldByName($field->getName() . '[_ConfirmPassword]')->Value());
 	}
 
+	public function testHashHidden() {
+		$field = new ConfirmedPasswordField('Password', 'Password', 'valueA');
+		$field->setCanBeEmpty(true);
+
+		$this->assertEquals('valueA', $field->Value());
+		$this->assertEquals('valueA', $field->children->fieldByName($field->getName() . '[_Password]')->Value());
+		$this->assertEquals('valueA', $field->children->fieldByName($field->getName() . '[_ConfirmPassword]')->Value());
+
+		$member = new Member();
+		$member->Password = "valueB";
+		$member->write();
+
+		$form = new Form($this, 'Form', new FieldList($field), new FieldList());
+		$form->loadDataFrom($member);
+
+		$this->assertEquals('', $field->Value());
+		$this->assertEquals('', $field->children->fieldByName($field->getName() . '[_Password]')->Value());
+		$this->assertEquals('', $field->children->fieldByName($field->getName() . '[_ConfirmPassword]')->Value());
+	}
+
 	public function testSetShowOnClick() {
 		//hide by default and display show/hide toggle button
 		$field = new ConfirmedPasswordField('Test', 'Testing', 'valueA', null, true);

From f47383f52e885ec526da7a5649e8d6e9a1d97294 Mon Sep 17 00:00:00 2001
From: Damian Mooyman 
Date: Thu, 20 Jun 2013 15:21:18 +1200
Subject: [PATCH 15/54] BUG Fixed issue where file upload via the HTML Editor
 media dialogue would not prompt users to overwrite existing files

---
 admin/css/screen.css          |  2 +-
 admin/scss/_style.scss        | 11 ++++++++---
 forms/HtmlEditorField.php     |  2 --
 javascript/HtmlEditorField.js |  5 +++--
 javascript/UploadField.js     | 13 +++++++++++--
 5 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/admin/css/screen.css b/admin/css/screen.css
index 141d99b45..3d62db2fb 100644
--- a/admin/css/screen.css
+++ b/admin/css/screen.css
@@ -668,7 +668,7 @@ body.cms-dialog { overflow: auto; background: url("../images/textures/bg_cms_mai
 .htmleditorfield-mediaform .htmleditorfield-from-cms .ss-uploadfield h4 { float: left; margin-top: 4px; margin-bottom: 0; }
 .htmleditorfield-mediaform .htmleditorfield-from-cms .ss-uploadfield .middleColumn { margin-top: 16px; margin-left: 184px; }
 .htmleditorfield-mediaform .htmleditorfield-from-cms .ss-uploadfield .field.treedropdown { border-bottom: 0; padding: 0; }
-.htmleditorfield-mediaform .ss-uploadfield-editandorganize { display: none; }
+.htmleditorfield-mediaform .ss-assetuploadfield .ss-uploadfield-editandorganize .ss-uploadfield-files .ss-uploadfield-item-info { background-color: #9e9e9e; background-image: url('data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0idXRmLTgiPz4gPHN2ZyB2ZXJzaW9uPSIxLjEiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyI+PGRlZnM+PGxpbmVhckdyYWRpZW50IGlkPSJncmFkIiBncmFkaWVudFVuaXRzPSJ1c2VyU3BhY2VPblVzZSIgeDE9IjUwJSIgeTE9IjAlIiB4Mj0iNTAlIiB5Mj0iMTAwJSI+PHN0b3Agb2Zmc2V0PSIwJSIgc3RvcC1jb2xvcj0iIzllOWU5ZSIvPjxzdG9wIG9mZnNldD0iOCUiIHN0b3AtY29sb3I9IiM5ZDlkOWQiLz48c3RvcCBvZmZzZXQ9IjUwJSIgc3RvcC1jb2xvcj0iIzg3ODc4NyIvPjxzdG9wIG9mZnNldD0iNTQlIiBzdG9wLWNvbG9yPSIjODY4Njg2Ii8+PHN0b3Agb2Zmc2V0PSI5NiUiIHN0b3AtY29sb3I9IiM2YjZiNmIiLz48c3RvcCBvZmZzZXQ9IjEwMCUiIHN0b3AtY29sb3I9IiM2YzZjNmMiLz48L2xpbmVhckdyYWRpZW50PjwvZGVmcz48cmVjdCB4PSIwIiB5PSIwIiB3aWR0aD0iMTAwJSIgaGVpZ2h0PSIxMDAlIiBmaWxsPSJ1cmwoI2dyYWQpIiAvPjwvc3ZnPiA='); background-size: 100%; background-image: -webkit-gradient(linear, 50% 0%, 50% 100%, color-stop(0%, #9e9e9e), color-stop(8%, #9d9d9d), color-stop(50%, #878787), color-stop(54%, #868686), color-stop(96%, #6b6b6b), color-stop(100%, #6c6c6c)); background-image: -webkit-linear-gradient(top, #9e9e9e 0%, #9d9d9d 8%, #878787 50%, #868686 54%, #6b6b6b 96%, #6c6c6c 100%); background-image: -moz-linear-gradient(top, #9e9e9e 0%, #9d9d9d 8%, #878787 50%, #868686 54%, #6b6b6b 96%, #6c6c6c 100%); background-image: -o-linear-gradient(top, #9e9e9e 0%, #9d9d9d 8%, #878787 50%, #868686 54%, #6b6b6b 96%, #6c6c6c 100%); background-image: linear-gradient(top, #9e9e9e 0%, #9d9d9d 8%, #878787 50%, #868686 54%, #6b6b6b 96%, #6c6c6c 100%); }
 
 /** -------------------------------------------- Search forms (used in AssetAdmin, ModelAdmin, etc) -------------------------------------------- */
 .cms-search-form { margin-bottom: 16px; }
diff --git a/admin/scss/_style.scss b/admin/scss/_style.scss
index 07c9ef271..1bd85cfaf 100644
--- a/admin/scss/_style.scss
+++ b/admin/scss/_style.scss
@@ -1535,9 +1535,14 @@ body.cms-dialog {
 		}
 	}
 
-	.ss-uploadfield-editandorganize {
-		display: none;
-	}	
+	.ss-assetuploadfield .ss-uploadfield-editandorganize {
+		.ss-uploadfield-files {
+			.ss-uploadfield-item-info {
+				background-color: grayscale(#5db4df);
+				@include background-image(linear-gradient(top,  grayscale(#5db4df) 0%, grayscale(#5db1dd) 8%, grayscale(#439bcb) 50%, grayscale(#3f99cd) 54%, grayscale(#207db6) 96%, grayscale(#1e7cba) 100%));
+			}
+		}
+	}
 }
 
 /** --------------------------------------------
diff --git a/forms/HtmlEditorField.php b/forms/HtmlEditorField.php
index c2e87f102..c5a9fc1a3 100644
--- a/forms/HtmlEditorField.php
+++ b/forms/HtmlEditorField.php
@@ -428,8 +428,6 @@ class HtmlEditorField_Toolbar extends RequestHandler {
 		$computerUploadField->removeExtraClass('ss-uploadfield');
 		$computerUploadField->setTemplate('HtmlEditorField_UploadField');
 		$computerUploadField->setFolderName(Config::inst()->get('Upload', 'uploads_folder'));
-		// @todo - Remove this once this field supports display and recovery of file upload validation errors
-		$computerUploadField->setOverwriteWarning(false);
 
 		$tabSet = new TabSet(
 			"MediaFormInsertMediaTabs",
diff --git a/javascript/HtmlEditorField.js b/javascript/HtmlEditorField.js
index dc6c443b9..055160ebc 100644
--- a/javascript/HtmlEditorField.js
+++ b/javascript/HtmlEditorField.js
@@ -808,7 +808,7 @@ ss.editorWrappers['default'] = ss.editorWrappers.tinyMCE;
 					});
 
 					ed.repaint();
-				})
+				});
 
 				this.getDialog().close();
 				return false;
@@ -926,8 +926,9 @@ ss.editorWrappers['default'] = ss.editorWrappers.tinyMCE;
 				var uploadedFiles = $('.ss-uploadfield-files', this).children('.ss-uploadfield-item');
 				uploadedFiles.each(function(){
 					var uploadedID = $(this).data('fileid');
-					if ($.inArray(uploadedID, editFieldIDs) == -1) {
+					if (uploadedID && $.inArray(uploadedID, editFieldIDs) == -1) {
 						//trigger the detail view for filling out details about the file we are about to insert into TinyMCE
+						$(this).remove(); // Remove successfully added item from the queue
 						form.showFileView(uploadedID);
 					}
 				});
diff --git a/javascript/UploadField.js b/javascript/UploadField.js
index 92362c3a6..1d6715732 100644
--- a/javascript/UploadField.js
+++ b/javascript/UploadField.js
@@ -64,13 +64,16 @@
 								.addClass('ui-state-warning-text');
 							data.context.find('.ss-uploadfield-item-progress').hide();
 							data.context.find('.ss-uploadfield-item-overwrite').show();
-							data.context.find('.ss-uploadfield-item-overwrite-warning').on('click', function(){
+							data.context.find('.ss-uploadfield-item-overwrite-warning').on('click', function(e){
 								data.context.find('.ss-uploadfield-item-progress').show();
 								data.context.find('.ss-uploadfield-item-overwrite').hide();
 								data.context.find('.ss-uploadfield-item-status')
 									.removeClass('ui-state-warning-text');
 								//upload only if the "overwrite" button is clicked
 								$.blueimpUI.fileupload.prototype._onSend.call(that, e, data);
+								
+								e.preventDefault(); // Avoid a form submit
+								return false;
 							});
 						} else {    //regular file upload
 							return $.blueimpUI.fileupload.prototype._onSend.call(that, e, data);
@@ -319,12 +322,14 @@
 		$('div.ss-upload .ss-uploadfield-startall').entwine({
 			onclick: function(e) {
 				this.closest('.ss-upload').find('.ss-uploadfield-item-start button').click();
+				e.preventDefault(); // Avoid a form submit
 				return false;
 			}
 		});
 		$('div.ss-upload .ss-uploadfield-item-cancelfailed').entwine({
 			onclick: function(e) {
 				this.closest('.ss-uploadfield-item').remove();
+				e.preventDefault(); // Avoid a form submit
 				return false;
 			}
 		});
@@ -349,6 +354,7 @@
 					fileupload._trigger('destroy', e, {context: item});	
 				}
 				
+				e.preventDefault(); // Avoid a form submit
 				return false;
 			}
 		});
@@ -371,6 +377,7 @@
 				}
 
 				e.preventDefault(); // Avoid a form submit
+				return false;
 			} 
 		});
 		$( 'div.ss-upload:not(.disabled):not(.readonly) .ss-uploadfield-item-edit').entwine({
@@ -403,6 +410,7 @@
 					editform.toggleEditForm();
 				}
 				e.preventDefault(); // Avoid a form submit
+				return false;
 			}
 		});
 
@@ -486,8 +494,9 @@
 		});
 		$('div.ss-upload .ss-uploadfield-fromfiles').entwine({
 			onclick: function(e) {
-				e.preventDefault();
 				this.getUploadField().openSelectDialog(this.closest('.ss-uploadfield-item'));
+				e.preventDefault(); // Avoid a form submit
+				return false;
 			}
 		});
 	});

From d1756a5a58f4c3000f9777feb4a39dc7889c4061 Mon Sep 17 00:00:00 2001
From: Will Rossiter 
Date: Thu, 20 Jun 2013 18:35:12 +1200
Subject: [PATCH 16/54] Update simple-contact-form.md

---
 docs/en/howto/simple-contact-form.md | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/en/howto/simple-contact-form.md b/docs/en/howto/simple-contact-form.md
index 73facd0d7..8a37fd250 100644
--- a/docs/en/howto/simple-contact-form.md
+++ b/docs/en/howto/simple-contact-form.md
@@ -44,7 +44,7 @@ First we create all the fields we want in the contact form, and put them inside
 We then create a `[api:FieldList]` of the form actions, or the buttons that submit the form. Here we add a single form action, with the name 'submit', and the label 'Submit'. We'll use the name of the form action later.
 
 	:::php
-	return new Form('Form', $this, $fields, $actions);
+	return new Form($this, 'Form', $fields, $actions);
 
 Finally we create the `Form` object and return it. The first argument is the name of the form – this has to be the same as the name of the function that creates the form, so we've used 'Form'. The second argument is the controller that the form is on – this is almost always $this. The third and fourth arguments are the fields and actions we created earlier.
 
@@ -61,7 +61,7 @@ Now that we have a contact form, we need some way of collecting the data submitt
 
 	:::php
 	class ContactPage_Controller extends Page_Controller {
-		static $allowed_actions = array('Form');
+		private static $allowed_actions = array('Form');
 		public function Form() {
 			// ...
 		}

From 53a2dbd20787cbd1ac486233a4b6c6cc072228b9 Mon Sep 17 00:00:00 2001
From: Mateusz Uzdowski 
Date: Fri, 21 Jun 2013 10:56:00 +1200
Subject: [PATCH 17/54] Add a note on the unit of the Session.timeout.

---
 control/Session.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/control/Session.php b/control/Session.php
index 20a9c3905..8c24d3ba5 100644
--- a/control/Session.php
+++ b/control/Session.php
@@ -86,7 +86,7 @@
 class Session {
 
 	/**
-	 * @var $timeout Set session timeout
+	 * @var $timeout Set session timeout in seconds.
 	 * @config
 	 */
 	private static $timeout = 0;

From b52087105c72abee4fea8b5e57942767e7318d38 Mon Sep 17 00:00:00 2001
From: CheeseSucker 
Date: Tue, 18 Jun 2013 22:11:32 +0300
Subject: [PATCH 18/54] FIX: ViewableData::obj() would sometimes return an
 empty object

For instance, this happens when these criteria are met:
  1) No casting has been specified for a method in $casting.
  2) A template accesses the field without any casting
  3) Any casts by the template will now yield an empty object.

After a brief look at the commit history, it can seem like this bug is several years old, unless it is a side-effect of other changes in the code.

== Steps to reproduce ==
Add two methods to be accessed by a template. Make sure you do not define an entry in $casting for them:
	public function Testus() {
		return "Tet1";
	}

	public function Testus2() {
		return "Tet2";
	}

Add this to a template:
	

First access:
"$Testus" : "$Testus.XML"
"$Testus2.XML" : "$Testus2"

Second access:
"$Testus" : "$Testus.XML"
"$Testus2.XML" : "$Testus2"

Open the page in a browser, and you will get: First access: "Tet1" : "" "Tet2" : "Tet2" Second access: "Tet1" : "" "" : "Tet2" We see that any cast can yield an empty string. --- view/ViewableData.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/view/ViewableData.php b/view/ViewableData.php index dc428d24f..7a6cf5b4c 100644 --- a/view/ViewableData.php +++ b/view/ViewableData.php @@ -383,7 +383,9 @@ class ViewableData extends Object implements IteratorAggregate { if(!is_object($value) && $forceReturnedObject) { $default = Config::inst()->get('ViewableData', 'default_cast', Config::FIRST_SET); - $value = new $default($fieldName); + $castedValue = new $default($fieldName); + $castedValue->setValue($value); + $value = $castedValue; } return $value; From 761eec773646f3ec0f62bec2827b806d77520a4c Mon Sep 17 00:00:00 2001 From: CheeseSucker Date: Wed, 19 Jun 2013 00:47:47 +0200 Subject: [PATCH 19/54] Unit test for bugfix in ViewableData::obj(). --- tests/view/ViewableDataTest.php | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/view/ViewableDataTest.php b/tests/view/ViewableDataTest.php index a90c165fe..8de0d715d 100644 --- a/tests/view/ViewableDataTest.php +++ b/tests/view/ViewableDataTest.php @@ -121,6 +121,28 @@ class ViewableDataTest extends SapphireTest { ); } } + + public function testObjWithCachedStringValueReturnsValidObject() { + $obj = new ViewableDataTest_NoCastingInformation(); + + // Save a literal string into cache + $cache = true; + $uncastedData = $obj->obj('noCastingInformation', null, false, $cache); + + // Fetch the cached string as an object + $forceReturnedObject = true; + $castedData = $obj->obj('noCastingInformation', null, $forceReturnedObject); + + // Uncasted data should always be the nonempty string + $this->assertNotEmpty($uncastedData, 'Uncasted data was empty.'); + $this->assertTrue(is_string($uncastedData), 'Uncasted data should be a string.'); + + // Casted data should be the string wrapped in a DBField-object. + $this->assertNotEmpty($castedData, 'Casted data was empty.'); + $this->assertInstanceOf('DBField', $castedData, 'Casted data should be instance of DBField.'); + + $this->assertEquals($uncastedData, $castedData->getValue(), 'Casted and uncasted strings are not equal.'); + } } /**#@+ @@ -212,4 +234,10 @@ class ViewableDataTest_CastingClass extends ViewableData { ); } +class ViewableDataTest_NoCastingInformation extends ViewableData { + public function noCastingInformation() { + return "No casting information"; + } +} + /**#@-*/ From e6bfabfd6cabcb1f0f79fb11b1e712597f60fe9c Mon Sep 17 00:00:00 2001 From: Jeremy Thomerson Date: Fri, 21 Jun 2013 16:19:06 +0000 Subject: [PATCH 20/54] TEST: additional test for ViewableData not wrapping cached strings --- tests/view/ViewableDataTest.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/view/ViewableDataTest.php b/tests/view/ViewableDataTest.php index 8de0d715d..855520691 100644 --- a/tests/view/ViewableDataTest.php +++ b/tests/view/ViewableDataTest.php @@ -81,6 +81,19 @@ class ViewableDataTest extends SapphireTest { $this->assertEquals('casted', $newViewableData->forTemplate()); } + public function testDefaultValueWrapping() { + $data = new ArrayData(array('Title' => 'SomeTitleValue')); + // this results in a cached raw string in ViewableData: + $this->assertTrue($data->hasValue('Title')); + $this->assertFalse($data->hasValue('SomethingElse')); + // this should cast the raw string to a StringField since we are + // passing true as the third argument: + $obj = $data->obj('Title', null, true); + $this->assertTrue(is_object($obj)); + // and the string field should have the value of the raw string: + $this->assertEquals('SomeTitleValue', $obj->forTemplate()); + } + public function testRAWVal() { $data = new ViewableDataTest_Castable(); $data->test = 'This & This'; From 6e7cae50fd9966da0e07bcdd174234b58b6e1eb9 Mon Sep 17 00:00:00 2001 From: ARNHOE Date: Sat, 22 Jun 2013 18:08:25 +0200 Subject: [PATCH 21/54] Updated helplink to 3.1 --- admin/code/LeftAndMain.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/admin/code/LeftAndMain.php b/admin/code/LeftAndMain.php index c57341d26..02cf200b0 100644 --- a/admin/code/LeftAndMain.php +++ b/admin/code/LeftAndMain.php @@ -76,7 +76,7 @@ class LeftAndMain extends Controller implements PermissionProvider { * @config * @var string */ - private static $help_link = 'http://3.0.userhelp.silverstripe.org'; + private static $help_link = 'http://userhelp.silverstripe.org/framework/en/3.1'; /** * @var array From 49835c3bb14e501ed71f925751e987c98d33de43 Mon Sep 17 00:00:00 2001 From: micmania1 Date: Sun, 23 Jun 2013 13:20:38 +0100 Subject: [PATCH 22/54] Updated calls to methods instead of firect properties in PaginatedList --- core/PaginatedList.php | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/core/PaginatedList.php b/core/PaginatedList.php index e4c8139be..3cdd456b5 100644 --- a/core/PaginatedList.php +++ b/core/PaginatedList.php @@ -80,7 +80,7 @@ class PaginatedList extends SS_ListDecorator { * @param int $page */ public function setCurrentPage($page) { - $this->pageStart = ($page - 1) * $this->pageLength; + $this->pageStart = ($page - 1) * $this->getPageLength(); return $this; } @@ -91,8 +91,8 @@ class PaginatedList extends SS_ListDecorator { */ public function getPageStart() { if ($this->pageStart === null) { - if ($this->request && isset($this->request[$this->getVar])) { - $this->pageStart = (int) $this->request[$this->getVar]; + if ($this->request && isset($this->request[$this->getPaginationGetVar()])) { + $this->pageStart = (int) $this->request[$this->getPaginationGetVar()]; } else { $this->pageStart = 0; } @@ -181,7 +181,7 @@ class PaginatedList extends SS_ListDecorator { if($this->limitItems) { $tmptList = clone $this->list; return new IteratorIterator( - $tmptList->limit($this->pageLength, $this->getPageStart()) + $tmptList->limit($this->getPageLength(), $this->getPageStart()) ); } else { return new IteratorIterator($this->list); @@ -223,7 +223,7 @@ class PaginatedList extends SS_ListDecorator { for ($i = $start; $i < $end; $i++) { $result->push(new ArrayData(array( 'PageNum' => $i + 1, - 'Link' => HTTP::setGetVar($this->getVar, $i * $this->pageLength), + 'Link' => HTTP::setGetVar($this->getPaginationGetVar(), $i * $this->getPageLength()), 'CurrentBool' => $this->CurrentPage() == ($i + 1) ))); } @@ -292,7 +292,7 @@ class PaginatedList extends SS_ListDecorator { } for ($i = 0; $i < $total; $i++) { - $link = HTTP::setGetVar($this->getVar, $i * $this->pageLength); + $link = HTTP::setGetVar($this->getPaginationGetVar(), $i * $this->getPageLength()); $num = $i + 1; $emptyRange = $num != 1 && $num != $total && ( @@ -321,14 +321,14 @@ class PaginatedList extends SS_ListDecorator { * @return int */ public function CurrentPage() { - return floor($this->getPageStart() / $this->pageLength) + 1; + return floor($this->getPageStart() / $this->getPageLength()) + 1; } /** * @return int */ public function TotalPages() { - return ceil($this->getTotalItems() / $this->pageLength); + return ceil($this->getTotalItems() / $this->getPageLength()); } /** @@ -369,9 +369,9 @@ class PaginatedList extends SS_ListDecorator { */ public function LastItem() { if ($start = $this->getPageStart()) { - return min($start + $this->pageLength, $this->getTotalItems()); + return min($start + $this->getPageLength(), $this->getTotalItems()); } else { - return min($this->pageLength, $this->getTotalItems()); + return min($this->getPageLength(), $this->getTotalItems()); } } @@ -381,7 +381,7 @@ class PaginatedList extends SS_ListDecorator { * @return string */ public function FirstLink() { - return HTTP::setGetVar($this->getVar, 0); + return HTTP::setGetVar($this->getPaginationGetVar(), 0); } /** @@ -390,7 +390,7 @@ class PaginatedList extends SS_ListDecorator { * @return string */ public function LastLink() { - return HTTP::setGetVar($this->getVar, ($this->TotalPages() - 1) * $this->pageLength); + return HTTP::setGetVar($this->getPaginationGetVar(), ($this->TotalPages() - 1) * $this->getPageLength()); } /** @@ -401,7 +401,7 @@ class PaginatedList extends SS_ListDecorator { */ public function NextLink() { if ($this->NotLastPage()) { - return HTTP::setGetVar($this->getVar, $this->getPageStart() + $this->pageLength); + return HTTP::setGetVar($this->getPaginationGetVar(), $this->getPageStart() + $this->getPageLength()); } } @@ -413,8 +413,8 @@ class PaginatedList extends SS_ListDecorator { */ public function PrevLink() { if ($this->NotFirstPage()) { - return HTTP::setGetVar($this->getVar, $this->getPageStart() - $this->pageLength); + return HTTP::setGetVar($this->getPaginationGetVar(), $this->getPageStart() - $this->getPageLength()); } } - } +} From d8b106e6ee3b75561b67cdd11712432ec959ec38 Mon Sep 17 00:00:00 2001 From: Craig Weber Date: Fri, 26 Oct 2012 20:54:08 +0000 Subject: [PATCH 23/54] FIX: TestRunner was not cleaning up DB on failure When a unit test being run by PHPUnit encountered a fatal error, TestRunner::tearDown was never being called. This resulted in tmpdb schemas littering the database from failed test runs. This changeset fixes the issue by registering TestRunner::tearDown as a shutdown function, so that it gets called even in the event of a PHP Fatal Error. --- dev/TestRunner.php | 6 ++++++ 1 file changed, 6 insertions(+) mode change 100644 => 100755 dev/TestRunner.php diff --git a/dev/TestRunner.php b/dev/TestRunner.php old mode 100644 new mode 100755 index d186ae87e..34cc423dc --- a/dev/TestRunner.php +++ b/dev/TestRunner.php @@ -315,6 +315,12 @@ class TestRunner extends Controller { $phpunitwrapper->setSuite($suite); $phpunitwrapper->setCoverageStatus($coverage); + // Make sure TearDown is called (even in the case of a fatal error) + $self = $this; + register_shutdown_function(function() use ($self) { + $self->tearDown(); + }); + $phpunitwrapper->runTests(); // get results of the PhpUnitWrapper class From 7340da03a710f205c291d64cf6b2078eb4ebe6bc Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 24 Jun 2013 13:39:05 +1200 Subject: [PATCH 24/54] Controller::redirect now returns the resulting SS_HTTPResponse, allowing the function to better support chaining --- control/Controller.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/control/Controller.php b/control/Controller.php index b07c804c0..e66a6f3c9 100644 --- a/control/Controller.php +++ b/control/Controller.php @@ -458,6 +458,8 @@ class Controller extends RequestHandler implements TemplateGlobalProvider { /** * Redirect to the given URL. + * + * @return SS_HTTPResponse */ public function redirect($url, $code=302) { if(!$this->response) $this->response = new SS_HTTPResponse(); @@ -473,7 +475,7 @@ class Controller extends RequestHandler implements TemplateGlobalProvider { $url = Director::baseURL() . $url; } - $this->response->redirect($url, $code); + return $this->response->redirect($url, $code); } /** From fb784af7380a06b6ae078f7af17e61acb0b3a2f6 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Thu, 20 Jun 2013 11:40:55 +0200 Subject: [PATCH 25/54] API Enforce $allowed_actions in RequestHandler->checkAccessAction() See discussion at https://groups.google.com/forum/?fromgroups#!topic/silverstripe-dev/Dodomh9QZjk Fixes an access issue where all public methods on FormField were allowed, and not checked for $allowed_actions. Before this patch you could e.g. call FormField->Value() on the first field by using action_Value. Removes the following assertion because it only worked due to RequestHandlingTest_AllowedControllerExtension *not* having $allowed_extensions declared: "Actions on magic methods are only accessible if explicitly allowed on the controller." --- admin/code/CMSProfileController.php | 1 + control/RequestHandler.php | 16 ++++++++-- docs/en/changelogs/3.1.0.md | 12 +++++--- docs/en/topics/controller.md | 21 ++------------ forms/Form.php | 19 ++++++++++-- forms/UploadField.php | 6 ++++ forms/gridfield/GridFieldDetailForm.php | 6 ++++ tests/control/ControllerTest.php | 20 +++++++++++-- tests/control/DirectorTest.php | 7 +++++ tests/control/RequestHandlingTest.php | 29 ++++++++++++------- tests/forms/EmailFieldTest.php | 2 ++ tests/forms/FormTest.php | 9 ++++++ .../GridFieldAddExistingAutocompleterTest.php | 2 ++ .../gridfield/GridFieldDetailFormTest.php | 10 +++++++ .../gridfield/GridField_URLHandlerTest.php | 11 +++++++ tests/security/SecurityTest.php | 3 ++ 16 files changed, 135 insertions(+), 39 deletions(-) diff --git a/admin/code/CMSProfileController.php b/admin/code/CMSProfileController.php index 9c35b009d..e1b776541 100644 --- a/admin/code/CMSProfileController.php +++ b/admin/code/CMSProfileController.php @@ -6,6 +6,7 @@ class CMSProfileController extends LeftAndMain { private static $menu_title = 'My Profile'; private static $required_permission_codes = false; + private static $tree_class = 'Member'; public function getResponseNegotiator() { diff --git a/control/RequestHandler.php b/control/RequestHandler.php index a6db29fe4..06604c914 100644 --- a/control/RequestHandler.php +++ b/control/RequestHandler.php @@ -96,6 +96,16 @@ class RequestHandler extends ViewableData { * @config */ private static $allowed_actions = null; + + /** + * @config + * @var boolean Enforce presence of $allowed_actions when checking acccess. + * Defaults to TRUE, meaning all URL actions will be denied. + * When set to FALSE, the controller will allow *all* public methods to be called. + * In most cases this isn't desireable, and in fact a security risk, + * since some helper methods can cause side effects which shouldn't be exposed through URLs. + */ + private static $require_allowed_actions = true; public function __construct() { $this->brokenOnConstruct = false; @@ -430,12 +440,12 @@ class RequestHandler extends ViewableData { // If defined as empty array, deny action $isAllowed = false; } elseif($allowedActions === null) { - // If undefined, allow action - $isAllowed = true; + // If undefined, allow action based on configuration + $isAllowed = !Config::inst()->get('RequestHandler', 'require_allowed_actions'); } // If we don't have a match in allowed_actions, - // whitelist the 'index' action as well as undefined actions. + // whitelist the 'index' action as well as undefined actions based on configuration. if(!$isDefined && ($action == 'index' || empty($action))) { $isAllowed = true; } diff --git a/docs/en/changelogs/3.1.0.md b/docs/en/changelogs/3.1.0.md index 2030715e3..f6c6e05e4 100644 --- a/docs/en/changelogs/3.1.0.md +++ b/docs/en/changelogs/3.1.0.md @@ -219,8 +219,7 @@ For more information about how to use the config system, see the ["Configuration In order to make controller access checks more consistent and easier to understand, the routing will require definition of `$allowed_actions` -on your own `Controller` subclasses if they contain any actions -accessible through URLs, or any forms. +on your own `Controller` subclasses if they contain any actions accessible through URLs. :::php class MyController extends Controller { @@ -228,8 +227,13 @@ accessible through URLs, or any forms. public function myaction($request) {} } -Please review all rules governing allowed actions in the -["controller" topic](/topics/controller). +You can overwrite the default behaviour on undefined `$allowed_actions` to allow all actions, +by setting the `RequestHandler.require_allowed_actions` config value to `false` (not recommended). + +This applies to anything extending `RequestHandler`, so please check your `Form` and `FormField` +subclasses as well. Keep in mind, action methods as denoted through `FormAction` names should NOT +be mentioned in `$allowed_actions` to avoid CSRF issues. +Please review all rules governing allowed actions in the ["controller" topic](/topics/controller). ### Removed support for "*" rules in `Controller::$allowed_actions` diff --git a/docs/en/topics/controller.md b/docs/en/topics/controller.md index 81880c948..a8c90ddc2 100644 --- a/docs/en/topics/controller.md +++ b/docs/en/topics/controller.md @@ -87,9 +87,7 @@ way to perform checks against permission codes or custom logic. There's a couple of rules guiding these checks: * Each class is only responsible for access control on the methods it defines - * If `$allowed_actions` is defined as an empty array, no actions are allowed - * If `$allowed_actions` is undefined, all public methods on the specific class are allowed - (not recommended) + * If `$allowed_actions` is an empty array or undefined, only the `index` action is allowed * Access checks on parent classes need to be overwritten via the Config API * Only public methods can be made accessible * If a method on a parent class is overwritten, access control for it has to be redefined as well @@ -102,6 +100,8 @@ There's a couple of rules guiding these checks: * `$allowed_actions` can be defined on `Extension` classes applying to the controller. If the permission check fails, SilverStripe will return a "403 Forbidden" HTTP status. +You can overwrite the default behaviour on undefined `$allowed_actions` to allow all actions, +by setting the `RequestHandler.require_allowed_actions` config value to `false` (not recommended). ### Through the action @@ -173,21 +173,6 @@ through `/fastfood/drivethrough/` to use the same order function. 'drivethrough/$Action/$ID/$Name' => 'order' ); -## Access Control - -### Through $allowed_actions - - * If `$allowed_actions` is undefined, `null` or `array()`, no actions are accessible - * Each class is only responsible for access control on the methods it defines - * Access checks on parent classes need to be overwritten via the Config API - * Only public methods can be made accessible - * If a method on a parent class is overwritten, access control for it has to be redefined as well - * An action named "index" is whitelisted by default - * Methods returning forms also count as actions which need to be defined - * Form action methods (targets of `FormAction`) should NOT be included in `$allowed_actions`, - they're handled separately through the form routing (see the ["forms" topic](/topics/forms)) - * `$allowed_actions` can be defined on `Extension` classes applying to the controller. - ## URL Patterns The `[api:RequestHandler]` class will parse all rules you specify against the diff --git a/forms/Form.php b/forms/Form.php index 0dd6f7c3f..6f692da8e 100644 --- a/forms/Form.php +++ b/forms/Form.php @@ -149,6 +149,8 @@ class Form extends RequestHandler { */ protected $attributes = array(); + private static $allowed_actions = array('handleField', 'httpSubmission'); + /** * Create a new form, with the given fields an action buttons. * @@ -298,7 +300,7 @@ class Form extends RequestHandler { Form::set_current_action($funcName); $this->setButtonClicked($funcName); } - + // Permission checks (first on controller, then falling back to form) if( // Ensure that the action is actually a button or method on the form, @@ -360,6 +362,19 @@ class Form extends RequestHandler { return $this->httpError(404); } + public function checkAccessAction($action) { + return ( + parent::checkAccessAction($action) + // Always allow actions which map to buttons. See httpSubmission() for further access checks. + || $this->actions->dataFieldByName('action_' . $action) + // Always allow actions on fields + || ( + $field = $this->checkFieldsForAction($this->Fields(), $action) + && $field->checkAccessAction($action) + ) + ); + } + /** * Returns the appropriate response up the controller chain * if {@link validate()} fails (which is checked prior to executing any form actions). @@ -412,7 +427,7 @@ class Form extends RequestHandler { if($field = $this->checkFieldsForAction($field->FieldList(), $funcName)) { return $field; } - } elseif ($field->hasMethod($funcName)) { + } elseif ($field->hasMethod($funcName) && $field->checkAccessAction($funcName)) { return $field; } } diff --git a/forms/UploadField.php b/forms/UploadField.php index 0c6e668ad..895793142 100644 --- a/forms/UploadField.php +++ b/forms/UploadField.php @@ -1331,6 +1331,12 @@ class UploadField_ItemHandler extends RequestHandler { '' => 'index', ); + private static $allowed_actions = array( + 'delete', + 'edit', + 'EditForm', + ); + /** * @param UploadFIeld $parent * @param int $item diff --git a/forms/gridfield/GridFieldDetailForm.php b/forms/gridfield/GridFieldDetailForm.php index 6a6dd89f5..0c7dc2187 100644 --- a/forms/gridfield/GridFieldDetailForm.php +++ b/forms/gridfield/GridFieldDetailForm.php @@ -194,6 +194,12 @@ class GridFieldDetailForm implements GridField_URLHandler { * @subpackage fields-gridfield */ class GridFieldDetailForm_ItemRequest extends RequestHandler { + + private static $allowed_actions = array( + 'edit', + 'view', + 'ItemEditForm' + ); /** * diff --git a/tests/control/ControllerTest.php b/tests/control/ControllerTest.php index 1666750ec..c0e9bd090 100644 --- a/tests/control/ControllerTest.php +++ b/tests/control/ControllerTest.php @@ -53,15 +53,31 @@ class ControllerTest extends FunctionalTest { $response = $this->get("ControllerTest_UnsecuredController/index"); $this->assertEquals(200, $response->getStatusCode(), - 'Access granted on index action without $allowed_actions on defining controller, ' . + 'Access denied on index action without $allowed_actions on defining controller, ' . 'when called with an action in the URL' ); + Config::inst()->update('RequestHandler', 'require_allowed_actions', false); + $response = $this->get("ControllerTest_UnsecuredController/index"); + $this->assertEquals(200, $response->getStatusCode(), + 'Access granted on index action without $allowed_actions on defining controller, ' . + 'when called with an action in the URL, and explicitly allowed through config' + ); + Config::inst()->update('RequestHandler', 'require_allowed_actions', true); + + $response = $this->get("ControllerTest_UnsecuredController/method1"); + $this->assertEquals(403, $response->getStatusCode(), + 'Access denied on action without $allowed_actions on defining controller, ' . + 'when called without an action in the URL' + ); + + Config::inst()->update('RequestHandler', 'require_allowed_actions', false); $response = $this->get("ControllerTest_UnsecuredController/method1"); $this->assertEquals(200, $response->getStatusCode(), 'Access granted on action without $allowed_actions on defining controller, ' . - 'when called without an action in the URL' + 'when called without an action in the URL, and explicitly allowed through config' ); + Config::inst()->update('RequestHandler', 'require_allowed_actions', true); $response = $this->get("ControllerTest_AccessBaseController/"); $this->assertEquals(200, $response->getStatusCode(), diff --git a/tests/control/DirectorTest.php b/tests/control/DirectorTest.php index 7cd8d4cca..9568307e5 100644 --- a/tests/control/DirectorTest.php +++ b/tests/control/DirectorTest.php @@ -348,6 +348,13 @@ class DirectorTest extends SapphireTest { class DirectorTestRequest_Controller extends Controller implements TestOnly { + private static $allowed_actions = array( + 'returnGetValue', + 'returnPostValue', + 'returnRequestValue', + 'returnCookieValue', + ); + public function returnGetValue($request) { return $_GET['somekey']; } public function returnPostValue($request) { return $_POST['somekey']; } diff --git a/tests/control/RequestHandlingTest.php b/tests/control/RequestHandlingTest.php index 8cc2693ff..99e2989bc 100644 --- a/tests/control/RequestHandlingTest.php +++ b/tests/control/RequestHandlingTest.php @@ -103,10 +103,6 @@ class RequestHandlingTest extends FunctionalTest { } public function testDisallowedExtendedActions() { - /* Actions on magic methods are only accessible if explicitly allowed on the controller. */ - $response = Director::test("testGoodBase1/extendedMethod"); - $this->assertEquals(404, $response->getStatusCode()); - /* Actions on an extension are allowed because they specifically provided appropriate allowed_actions items */ $response = Director::test("testGoodBase1/otherExtendedMethod"); $this->assertEquals("otherExtendedMethod", $response->getBody()); @@ -119,9 +115,10 @@ class RequestHandlingTest extends FunctionalTest { $response = Director::test("RequestHandlingTest_AllowedController/failoverMethod"); $this->assertEquals("failoverMethod", $response->getBody()); - /* The action on the extension has also been explicitly allowed even though it wasn't on the extension */ + /* The action on the extension is allowed when explicitly allowed on extension, + even if its not mentioned in controller */ $response = Director::test("RequestHandlingTest_AllowedController/extendedMethod"); - $this->assertEquals("extendedMethod", $response->getBody()); + $this->assertEquals(200, $response->getStatusCode()); /* This action has been blocked by an argument to a method */ $response = Director::test('RequestHandlingTest_AllowedController/blockMethod'); @@ -420,9 +417,13 @@ class RequestHandlingTest_FormActionController extends Controller { * Simple extension for the test controller */ class RequestHandlingTest_ControllerExtension extends Extension { + public static $called_error = false; + public static $called_404_error = false; + private static $allowed_actions = array('extendedMethod'); + public function extendedMethod() { return "extendedMethod"; } @@ -454,7 +455,6 @@ class RequestHandlingTest_AllowedController extends Controller implements TestOn private static $allowed_actions = array( 'failoverMethod', // part of the failover object - 'extendedMethod', // part of the RequestHandlingTest_ControllerExtension object 'blockMethod' => '->provideAccess(false)', 'allowMethod' => '->provideAccess', ); @@ -536,6 +536,8 @@ class RequestHandlingTest_Form extends Form { } class RequestHandlingTest_ControllerFormWithAllowedActions extends Controller implements TestOnly { + + private static $allowed_actions = array('Form'); public function Form() { return new RequestHandlingTest_FormWithAllowedActions( @@ -543,8 +545,7 @@ class RequestHandlingTest_ControllerFormWithAllowedActions extends Controller im 'Form', new FieldList(), new FieldList( - new FormAction('allowedformaction'), - new FormAction('disallowedformaction') // disallowed through $allowed_actions in form + new FormAction('allowedformaction') ) ); } @@ -554,7 +555,6 @@ class RequestHandlingTest_FormWithAllowedActions extends Form { private static $allowed_actions = array( 'allowedformaction' => 1, - 'httpSubmission' => 1, // TODO This should be an exception on the parent class ); public function allowedformaction() { @@ -602,6 +602,9 @@ class RequestHandlingTest_FormField extends FormField { * Form field for the test */ class RequestHandlingTest_SubclassedFormField extends RequestHandlingTest_FormField { + + private static $allowed_actions = array('customSomething'); + // We have some url_handlers defined that override RequestHandlingTest_FormField handlers. // We will confirm that the url_handlers inherit. private static $url_handlers = array( @@ -619,6 +622,8 @@ class RequestHandlingTest_SubclassedFormField extends RequestHandlingTest_FormFi * Controller for the test */ class RequestHandlingFieldTest_Controller extends Controller implements TestOnly { + + private static $allowed_actions = array('TestForm'); public function TestForm() { return new Form($this, "TestForm", new FieldList( @@ -641,4 +646,8 @@ class RequestHandlingTest_HandlingField extends FormField { public function actionOnField() { return "Test method on $this->name"; } + + public function actionNotAllowedOnField() { + return "actionNotAllowedOnField on $this->name"; + } } diff --git a/tests/forms/EmailFieldTest.php b/tests/forms/EmailFieldTest.php index d60676211..caeb86154 100644 --- a/tests/forms/EmailFieldTest.php +++ b/tests/forms/EmailFieldTest.php @@ -73,6 +73,8 @@ class EmailFieldTest_Validator extends Validator { class EmailFieldTest_Controller extends Controller implements TestOnly { + private static $allowed_actions = array('Form'); + private static $url_handlers = array( '$Action//$ID/$OtherID' => "handleAction", ); diff --git a/tests/forms/FormTest.php b/tests/forms/FormTest.php index 428a63a13..36573da5f 100644 --- a/tests/forms/FormTest.php +++ b/tests/forms/FormTest.php @@ -458,6 +458,9 @@ class FormTest_Team extends DataObject implements TestOnly { } class FormTest_Controller extends Controller implements TestOnly { + + private static $allowed_actions = array('Form'); + private static $url_handlers = array( '$Action//$ID/$OtherID' => "handleAction", ); @@ -503,6 +506,9 @@ class FormTest_Controller extends Controller implements TestOnly { } class FormTest_ControllerWithSecurityToken extends Controller implements TestOnly { + + private static $allowed_actions = array('Form'); + private static $url_handlers = array( '$Action//$ID/$OtherID' => "handleAction", ); @@ -537,6 +543,9 @@ class FormTest_ControllerWithSecurityToken extends Controller implements TestOnl } class FormTest_ControllerWithStrictPostCheck extends Controller implements TestOnly { + + private static $allowed_actions = array('Form'); + protected $template = 'BlankPage'; public function Link($action = null) { diff --git a/tests/forms/gridfield/GridFieldAddExistingAutocompleterTest.php b/tests/forms/gridfield/GridFieldAddExistingAutocompleterTest.php index 2141a7fcb..fdff0d0d9 100644 --- a/tests/forms/gridfield/GridFieldAddExistingAutocompleterTest.php +++ b/tests/forms/gridfield/GridFieldAddExistingAutocompleterTest.php @@ -97,6 +97,8 @@ class GridFieldAddExistingAutocompleterTest extends FunctionalTest { class GridFieldAddExistingAutocompleterTest_Controller extends Controller implements TestOnly { + private static $allowed_actions = array('Form'); + protected $template = 'BlankPage'; public function Form() { diff --git a/tests/forms/gridfield/GridFieldDetailFormTest.php b/tests/forms/gridfield/GridFieldDetailFormTest.php index 055badcda..ee729310d 100644 --- a/tests/forms/gridfield/GridFieldDetailFormTest.php +++ b/tests/forms/gridfield/GridFieldDetailFormTest.php @@ -282,6 +282,7 @@ class GridFieldDetailFormTest_PeopleGroup extends DataObject implements TestOnly } class GridFieldDetailFormTest_Category extends DataObject implements TestOnly { + private static $db = array( 'Name' => 'Varchar' ); @@ -306,6 +307,9 @@ class GridFieldDetailFormTest_Category extends DataObject implements TestOnly { } class GridFieldDetailFormTest_Controller extends Controller implements TestOnly { + + private static $allowed_actions = array('Form'); + protected $template = 'BlankPage'; public function Form() { @@ -326,6 +330,9 @@ class GridFieldDetailFormTest_Controller extends Controller implements TestOnly } class GridFieldDetailFormTest_GroupController extends Controller implements TestOnly { + + private static $allowed_actions = array('Form'); + protected $template = 'BlankPage'; public function Form() { @@ -339,6 +346,9 @@ class GridFieldDetailFormTest_GroupController extends Controller implements Test } class GridFieldDetailFormTest_CategoryController extends Controller implements TestOnly { + + private static $allowed_actions = array('Form'); + protected $template = 'BlankPage'; public function Form() { diff --git a/tests/forms/gridfield/GridField_URLHandlerTest.php b/tests/forms/gridfield/GridField_URLHandlerTest.php index ec7204b60..914e6e0a3 100644 --- a/tests/forms/gridfield/GridField_URLHandlerTest.php +++ b/tests/forms/gridfield/GridField_URLHandlerTest.php @@ -30,6 +30,9 @@ class GridField_URLHandlerTest extends FunctionalTest { } class GridField_URLHandlerTest_Controller extends Controller implements TestOnly { + + private static $allowed_actions = array('Form'); + public function Link() { return get_class($this) ."/"; } @@ -51,6 +54,9 @@ class GridField_URLHandlerTest_Controller extends Controller implements TestOnly * Test URLHandler with a nested request handler */ class GridField_URLHandlerTest_Component extends RequestHandler implements GridField_URLHandler { + + private static $allowed_actions = array('Form', 'showform', 'testpage', 'handleItem'); + protected $gridField; public function getURLHandlers($gridField) { @@ -96,8 +102,13 @@ class GridField_URLHandlerTest_Component extends RequestHandler implements GridF } class GridField_URLHandlerTest_Component_ItemRequest extends RequestHandler { + + private static $allowed_actions = array('Form', 'showform', 'testpage'); + protected $gridField; + protected $link; + protected $id; public function __construct($gridField, $id, $link) { diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php index d23246db5..294b082b4 100644 --- a/tests/security/SecurityTest.php +++ b/tests/security/SecurityTest.php @@ -439,6 +439,9 @@ class SecurityTest extends FunctionalTest { } class SecurityTest_SecuredController extends Controller implements TestOnly { + + private static $allowed_actions = array('index'); + public function index() { if(!Permission::check('ADMIN')) return Security::permissionFailure($this); From 8c9ef8feb9c7685e0e2b32353c1eab05a04fb3a7 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Mon, 24 Jun 2013 16:12:00 +0200 Subject: [PATCH 26/54] "Insert Media" dialog: Reposition separately (fixes #783) FF21 and IE10 seem to propagate the DOM attribute changes differently from Chrome: The dimensions can't be set in the same setOptions() call through jQuery UI here. Fixed this by a separate setOption() call. --- admin/javascript/ssui.core.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/admin/javascript/ssui.core.js b/admin/javascript/ssui.core.js index dff784fe7..de5b8a0df 100644 --- a/admin/javascript/ssui.core.js +++ b/admin/javascript/ssui.core.js @@ -207,12 +207,8 @@ opts.height = newHeight; } } - if(this.options.autoPosition) { - opts.position = this.options.position; - } if(!jQuery.isEmptyObject(opts)) { - // Resize dialog this._setOptions(opts); // Resize iframe within dialog @@ -226,6 +222,11 @@ - parseFloat(this.element.css('paddingTop')) - parseFloat(this.element.css('paddingBottom')) ); + + // Enforce new position + if(this.options.autoPosition) { + this._setOption("position", this.options.position); + } } } }); From 1046530ff680b62495176972aa39c02bbe3541ba Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Mon, 24 Jun 2013 17:14:32 +0200 Subject: [PATCH 27/54] "Insert Media" dialog: Prevent loading indicator in IE8+ Fixes https://github.com/silverstripe/silverstripe-cms/issues/782 --- admin/javascript/ssui.core.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/admin/javascript/ssui.core.js b/admin/javascript/ssui.core.js index de5b8a0df..0b4261512 100644 --- a/admin/javascript/ssui.core.js +++ b/admin/javascript/ssui.core.js @@ -153,7 +153,7 @@ iframe.bind('load', function(e) { if($(this).attr('src') == 'about:blank') return; - $(this).show(); + iframe.addClass('loaded').show(); // more reliable than 'src' attr check (in IE) self._resizeIframe(); self.uiDialog.removeClass('loading'); }).hide(); @@ -170,7 +170,7 @@ var self = this, iframe = this.element.children('iframe'); // Load iframe - if(!iframe.attr('src') || this.options.reloadOnOpen) { + if(this.options.iframeUrl && (!iframe.hasClass('loaded') || this.options.reloadOnOpen)) { iframe.hide(); iframe.attr('src', this.options.iframeUrl); this.uiDialog.addClass('loading'); From 18299322bc6dc9af017e2c6d3f4272cdacc586fe Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Mon, 24 Jun 2013 18:58:46 +0200 Subject: [PATCH 28/54] "Insert Media" dialog: Fixed event names Fixes https://github.com/silverstripe/silverstripe-cms/issues/781 Regression from 9f600ada which uses jQuery UI widget "subclass" that also affects event names. --- javascript/HtmlEditorField.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/javascript/HtmlEditorField.js b/javascript/HtmlEditorField.js index 055160ebc..90cd41acf 100644 --- a/javascript/HtmlEditorField.js +++ b/javascript/HtmlEditorField.js @@ -382,7 +382,7 @@ ss.editorWrappers['default'] = ss.editorWrappers.tinyMCE; success: function(html) { dialog.html(html); dialog.getForm().setElement(self); - dialog.trigger('dialogopen'); + dialog.trigger('ssdialogopen'); } }); } @@ -452,7 +452,7 @@ ss.editorWrappers['default'] = ss.editorWrappers.tinyMCE; }, fromDialog: { - ondialogopen: function(){ + onssdialogopen: function(){ var ed = this.getEditor(); ed.onopen(); @@ -467,7 +467,7 @@ ss.editorWrappers['default'] = ss.editorWrappers.tinyMCE; this.redraw(); }, - ondialogclose: function(){ + onssdialogclose: function(){ var ed = this.getEditor(); ed.onclose(); From ffc764dc3c45c7e9171715b92581a7c97d087785 Mon Sep 17 00:00:00 2001 From: jonom Date: Mon, 24 Jun 2013 14:25:16 -0600 Subject: [PATCH 29/54] NEW: Allow configuration of initial insertion width for images and media Moved default insertion dimensions logic from JS to PHP to allow setting through config API --- forms/HtmlEditorField.php | 68 ++++++++++++++++++++++++++++++----- javascript/HtmlEditorField.js | 3 -- 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/forms/HtmlEditorField.php b/forms/HtmlEditorField.php index c5a9fc1a3..5ce926636 100644 --- a/forms/HtmlEditorField.php +++ b/forms/HtmlEditorField.php @@ -14,6 +14,12 @@ class HtmlEditorField extends TextareaField { */ private static $use_gzip = true; + /** + * @config + * @var Integer Default insertion width for Images and Media + */ + private static $insert_width = 600; + protected $rows = 30; /** @@ -620,10 +626,10 @@ class HtmlEditorField_Toolbar extends RequestHandler { 'CSSClass', _t('HtmlEditorField.CSSCLASS', 'Alignment / style'), array( - 'left' => _t('HtmlEditorField.CSSCLASSLEFT', 'On the left, with text wrapping around.'), 'leftAlone' => _t('HtmlEditorField.CSSCLASSLEFTALONE', 'On the left, on its own.'), - 'right' => _t('HtmlEditorField.CSSCLASSRIGHT', 'On the right, with text wrapping around.'), 'center' => _t('HtmlEditorField.CSSCLASSCENTER', 'Centered, on its own.'), + 'left' => _t('HtmlEditorField.CSSCLASSLEFT', 'On the left, with text wrapping around.'), + 'right' => _t('HtmlEditorField.CSSCLASSRIGHT', 'On the right, with text wrapping around.') ) )->addExtraClass('last') ); @@ -634,12 +640,12 @@ class HtmlEditorField_Toolbar extends RequestHandler { TextField::create( 'Width', _t('HtmlEditorField.IMAGEWIDTHPX', 'Width'), - $file->Width + $file->InsertWidth )->setMaxLength(5), TextField::create( 'Height', _t('HtmlEditorField.IMAGEHEIGHTPX', 'Height'), - $file->Height + $file->InsertHeight )->setMaxLength(5) )->addExtraClass('dimensions last') ); @@ -744,10 +750,10 @@ class HtmlEditorField_Toolbar extends RequestHandler { 'CSSClass', _t('HtmlEditorField.CSSCLASS', 'Alignment / style'), array( - 'left' => _t('HtmlEditorField.CSSCLASSLEFT', 'On the left, with text wrapping around.'), 'leftAlone' => _t('HtmlEditorField.CSSCLASSLEFTALONE', 'On the left, on its own.'), - 'right' => _t('HtmlEditorField.CSSCLASSRIGHT', 'On the right, with text wrapping around.'), 'center' => _t('HtmlEditorField.CSSCLASSCENTER', 'Centered, on its own.'), + 'left' => _t('HtmlEditorField.CSSCLASSLEFT', 'On the left, with text wrapping around.'), + 'right' => _t('HtmlEditorField.CSSCLASSRIGHT', 'On the right, with text wrapping around.') ) )->addExtraClass('last') ); @@ -757,12 +763,12 @@ class HtmlEditorField_Toolbar extends RequestHandler { TextField::create( 'Width', _t('HtmlEditorField.IMAGEWIDTHPX', 'Width'), - $file->Width + $file->InsertWidth )->setMaxLength(5), TextField::create( 'Height', " x " . _t('HtmlEditorField.IMAGEHEIGHTPX', 'Height'), - $file->Height + $file->InsertHeight )->setMaxLength(5) )->addExtraClass('dimensions last') ); @@ -908,6 +914,29 @@ class HtmlEditorField_Embed extends HtmlEditorField_File { return $this->oembed->Height ?: 100; } + /** + * Provide an initial width for inserted media, restricted based on $embed_width + * + * @return int + */ + public function getInsertWidth() { + $width = $this->getWidth(); + $maxWidth = Config::inst()->get('HtmlEditorField', 'insert_width'); + return ($width <= $maxWidth) ? $width : $maxWidth; + } + + /** + * Provide an initial height for inserted media, scaled proportionally to the initial width + * + * @return int + */ + public function getInsertHeight() { + $width = $this->getWidth(); + $height = $this->getHeight(); + $maxWidth = Config::inst()->get('HtmlEditorField', 'insert_width'); + return ($width <= $maxWidth) ? $height : round($height*($maxWidth/$width)); + } + public function getPreview() { if(isset($this->oembed->thumbnail_url)) { return sprintf('', $this->oembed->thumbnail_url); @@ -964,6 +993,29 @@ class HtmlEditorField_Image extends HtmlEditorField_File { return ($this->file) ? $this->file->Height : $this->height; } + /** + * Provide an initial width for inserted image, restricted based on $embed_width + * + * @return int + */ + public function getInsertWidth() { + $width = $this->getWidth(); + $maxWidth = Config::inst()->get('HtmlEditorField', 'insert_width'); + return ($width <= $maxWidth) ? $width : $maxWidth; + } + + /** + * Provide an initial height for inserted image, scaled proportionally to the initial width + * + * @return int + */ + public function getInsertHeight() { + $width = $this->getWidth(); + $height = $this->getHeight(); + $maxWidth = Config::inst()->get('HtmlEditorField', 'insert_width'); + return ($width <= $maxWidth) ? $height : round($height*($maxWidth/$width)); + } + public function getPreview() { return ($this->file) ? $this->file->CMSThumbnail() : sprintf('', $this->url); } diff --git a/javascript/HtmlEditorField.js b/javascript/HtmlEditorField.js index 90cd41acf..0c150561a 100644 --- a/javascript/HtmlEditorField.js +++ b/javascript/HtmlEditorField.js @@ -1255,9 +1255,6 @@ ss.editorWrappers['default'] = ss.editorWrappers.tinyMCE; this.setOrigVal(parseInt(this.val(), 10)); - // Default to a managable size for the HTML view. Can be overwritten by user after initialization - if(this.attr('name') == 'Width') this.closest('.ss-htmleditorfield-file').updateDimensions('Width', 600); - }, onunmatch: function() { this._super(); From ae3e3f3b44ce1c01832222b257ca2c26966027dc Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Tue, 25 Jun 2013 17:35:16 +1200 Subject: [PATCH 30/54] FIX Arguments to method calls reseting scope --- tests/view/SSViewerTest.php | 69 ++++++++++++++++++++++++++++++++++- view/SSTemplateParser.php | 2 +- view/SSTemplateParser.php.inc | 2 +- view/SSViewer.php | 24 ++++++++++-- 4 files changed, 89 insertions(+), 8 deletions(-) diff --git a/tests/view/SSViewerTest.php b/tests/view/SSViewerTest.php index 15003e9d9..5e7589165 100644 --- a/tests/view/SSViewerTest.php +++ b/tests/view/SSViewerTest.php @@ -1074,7 +1074,7 @@ after') $origEnv = Config::inst()->get('Director', 'environment_type'); Config::inst()->update('Director', 'environment_type', 'dev'); Config::inst()->update('SSViewer', 'source_file_comments', true); - $f = FRAMEWORK_PATH . '/tests/templates/SSViewerTestComments'; + $f = FRAMEWORK_PATH . '/tests/templates/SSViewerTestComments'; $templates = array( array( 'name' => 'SSViewerTestCommentsFullSource', @@ -1090,7 +1090,8 @@ after') array( 'name' => 'SSViewerTestCommentsFullSourceHTML4Doctype', 'expected' => "" - . "" + . "" . "" . "" . "\t" @@ -1209,6 +1210,45 @@ after') "tests/forms/RequirementsTest_a.js" )); } + + public function testCallsWithArguments() { + $data = new ArrayData(array( + 'Set' => new ArrayList(array( + new SSViewerTest_Object("1"), + new SSViewerTest_Object("2"), + new SSViewerTest_Object("3"), + new SSViewerTest_Object("4"), + new SSViewerTest_Object("5"), + )), + 'Level' => new SSViewerTest_LevelTest(1), + 'Nest' => array( + 'Level' => new SSViewerTest_LevelTest(2), + ), + )); + + $tests = array( + '$Level.output(1)' => '1-1', + '$Nest.Level.output($Set.First.Number)' => '2-1', + '<% with $Set %>$Up.Level.output($First.Number)<% end_with %>' => '1-1', + '<% with $Set %>$Top.Nest.Level.output($First.Number)<% end_with %>' => '2-1', + '<% loop $Set %>$Up.Nest.Level.output($Number)<% end_loop %>' => '2-12-22-32-42-5', + '<% loop $Set %>$Top.Level.output($Number)<% end_loop %>' => '1-11-21-31-41-5', + '<% with $Nest %>$Level.output($Top.Set.First.Number)<% end_with %>' => '2-1', + '<% with $Level %>$output($Up.Set.Last.Number)<% end_with %>' => '1-5', + '<% with $Level.forWith($Set.Last.Number) %>$output("hi")<% end_with %>' => '5-hi', + '<% loop $Level.forLoop($Set.First.Number) %>$Number<% end_loop %>' => '!0', + '<% with $Nest %> + <% with $Level.forWith($Up.Set.First.Number) %>$output("hi")<% end_with %> + <% end_with %>' => '1-hi', + '<% with $Nest %> + <% loop $Level.forLoop($Top.Set.Last.Number) %>$Number<% end_loop %> + <% end_with %>' => '!0!1!2!3!4', + ); + + foreach($tests as $template => $expected) { + $this->assertEquals($expected, trim($this->render($template, $data))); + } + } } /** @@ -1347,3 +1387,28 @@ class SSViewerTest_GlobalProvider implements TemplateGlobalProvider, TestOnly { } } + +class SSViewerTest_LevelTest extends ViewableData implements TestOnly { + protected $depth; + + public function __construct($depth = 1) { + $this->depth = $depth; + } + + public function output($val) { + return "$this->depth-$val"; + } + + public function forLoop($number) { + $ret = array(); + for($i = 0; $i < (int)$number; ++$i) { + $ret[] = new SSViewerTest_Object("!$i"); + } + return new ArrayList($ret); + } + + public function forWith($number) { + return new self($number); + } +} + diff --git a/view/SSTemplateParser.php b/view/SSTemplateParser.php index c3e3532b6..efe7e27ea 100644 --- a/view/SSTemplateParser.php +++ b/view/SSTemplateParser.php @@ -615,7 +615,7 @@ class SSTemplateParser extends Parser { function Lookup__construct(&$res) { - $res['php'] = '$scope'; + $res['php'] = '$scope->locally()'; $res['LookupSteps'] = array(); } diff --git a/view/SSTemplateParser.php.inc b/view/SSTemplateParser.php.inc index 68249b3ed..3c1349efe 100644 --- a/view/SSTemplateParser.php.inc +++ b/view/SSTemplateParser.php.inc @@ -157,7 +157,7 @@ class SSTemplateParser extends Parser { */ function Lookup__construct(&$res) { - $res['php'] = '$scope'; + $res['php'] = '$scope->locally()'; $res['LookupSteps'] = array(); } diff --git a/view/SSViewer.php b/view/SSViewer.php index 220d59d87..40df8f510 100644 --- a/view/SSViewer.php +++ b/view/SSViewer.php @@ -49,18 +49,34 @@ class SSViewer_Scope { public function __construct($item){ $this->item = $item; - $this->localIndex=0; + $this->localIndex = 0; + $this->localStack = array(); $this->itemStack[] = array($this->item, null, 0, null, null, 0); } public function getItem(){ return $this->itemIterator ? $this->itemIterator->current() : $this->item; } - - public function resetLocalScope(){ + + /** Called at the start of every lookup chain by SSTemplateParser to indicate a new lookup from local scope */ + public function locally() { list($this->item, $this->itemIterator, $this->itemIteratorTotal, $this->popIndex, $this->upIndex, $this->currentIndex) = $this->itemStack[$this->localIndex]; - array_splice($this->itemStack, $this->localIndex+1); + + // Remember any un-completed (resetLocalScope hasn't been called) lookup chain. Even if there isn't an + // un-completed chain we need to store an empty item, as resetLocalScope doesn't know the difference later + $this->localStack[] = array_splice($this->itemStack, $this->localIndex+1); + + return $this; + } + + public function resetLocalScope(){ + $previousLocalState = $this->localStack ? array_pop($this->localStack) : null; + + array_splice($this->itemStack, $this->localIndex+1, count($this->itemStack), $previousLocalState); + + list($this->item, $this->itemIterator, $this->itemIteratorTotal, $this->popIndex, $this->upIndex, + $this->currentIndex) = end($this->itemStack); } public function getObj($name, $arguments = null, $forceReturnedObject = true, $cache = false, $cacheName = null) { From 2a4fd903167fb7186a0f99f32872447a0603a8b1 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Tue, 25 Jun 2013 10:35:30 +0200 Subject: [PATCH 31/54] Docs: Note about branch merging --- docs/en/misc/contributing/code.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/misc/contributing/code.md b/docs/en/misc/contributing/code.md index 6576a17e2..2bab00355 100644 --- a/docs/en/misc/contributing/code.md +++ b/docs/en/misc/contributing/code.md @@ -82,7 +82,7 @@ If you're familiar with it, here's the short version of what you need to know. O * **Choose the correct branch**: Assume the current release is 3.0.3, and 3.1.0 is in beta state. Most pull requests should go against the `3.1.x-dev` *pre-release branch*, only critical bugfixes against the `3.0.x-dev` *release branch*. If you're changing an API or introducing a major feature, - the pull request should go against `master` (read more about our [release process](/misc/release-process)). + the pull request should go against `master` (read more about our [release process](/misc/release-process)). Branches are periodically merged "upwards" (3.0 into 3.1, 3.1 into master). ### Editing files directly on GitHub.com From 755a95e3f771dab9bfb7441135e01231ad08c968 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Tue, 25 Jun 2013 15:21:55 +0200 Subject: [PATCH 32/54] FIX UploadField: IE10 single click for upload trigger Fixes https://github.com/silverstripe/silverstripe-cms/issues/644. See https://github.com/blueimp/jQuery-File-Upload/commit/d45deb15f4934f7a2bce03040c879aca8eaa9c8a --- css/UploadField.css | 2 +- scss/UploadField.scss | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/css/UploadField.css b/css/UploadField.css index 050a051db..5d6468336 100644 --- a/css/UploadField.css +++ b/css/UploadField.css @@ -43,4 +43,4 @@ Used in side panels and action tabs .ss-uploadfield .ss-uploadfield-addfile.borderTop { border-top: 1px solid #b3b3b3; } .ss-upload .clear { clear: both; } -.ss-upload .ss-uploadfield-fromcomputer input { /* since we can't really style the file input, we use this hack to make it as big as the button and hide it */ position: absolute; top: 0; right: 0; margin: 0; border: solid #000; border-width: 0 0 100px 200px; opacity: 0; filter: alpha(opacity=0); -o-transform: translate(250px, -50px) scale(1); -moz-transform: translate(-300px, 0) scale(4); direction: ltr; cursor: pointer; } +.ss-upload .ss-uploadfield-fromcomputer input { /* since we can't really style the file input, we use this hack to make it as big as the button and hide it */ position: absolute; top: 0; right: 0; margin: 0; opacity: 0; filter: alpha(opacity=0); transform: translate(-300px, 0) scale(4); font-size: 23px; direction: ltr; cursor: pointer; height: 30px; line-height: 30px; } diff --git a/scss/UploadField.scss b/scss/UploadField.scss index a56a994fb..01efe3ef7 100644 --- a/scss/UploadField.scss +++ b/scss/UploadField.scss @@ -217,14 +217,14 @@ top: 0; right: 0; margin: 0; - border: solid #000; - border-width: 0 0 100px 200px; opacity: 0; filter: alpha(opacity=0); - -o-transform: translate(250px, -50px) scale(1); - -moz-transform: translate(-300px, 0) scale(4); + transform: translate(-300px, 0) scale(4); + font-size: 23px; direction: ltr; cursor: pointer; + height: 30px; + line-height: 30px; } } } \ No newline at end of file From 83726b21a204bb18f4d5efe10e38858358766bd7 Mon Sep 17 00:00:00 2001 From: Will Morgan Date: Tue, 28 May 2013 15:25:18 +0200 Subject: [PATCH 33/54] Using extendedCan for can* --- model/DataObject.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/model/DataObject.php b/model/DataObject.php index 0b6109860..a0efd74c5 100644 --- a/model/DataObject.php +++ b/model/DataObject.php @@ -2581,6 +2581,10 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity * @return boolean */ public function canView($member = null) { + $extended = $this->extendedCan(__FUNCTION__, $member); + if($extended !== null) { + return $extended; + } return Permission::check('ADMIN', 'any', $member); } @@ -2589,6 +2593,10 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity * @return boolean */ public function canEdit($member = null) { + $extended = $this->extendedCan(__FUNCTION__, $member); + if($extended !== null) { + return $extended; + } return Permission::check('ADMIN', 'any', $member); } @@ -2597,6 +2605,10 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity * @return boolean */ public function canDelete($member = null) { + $extended = $this->extendedCan(__FUNCTION__, $member); + if($extended !== null) { + return $extended; + } return Permission::check('ADMIN', 'any', $member); } @@ -2607,6 +2619,10 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity * @return boolean */ public function canCreate($member = null) { + $extended = $this->extendedCan(__FUNCTION__, $member); + if($extended !== null) { + return $extended; + } return Permission::check('ADMIN', 'any', $member); } From 09b31c642f08567682a8b646e956de25b587153d Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Tue, 25 Jun 2013 16:33:00 +0200 Subject: [PATCH 34/54] Allow Form->forTemplate() URL access (fixes #788) Need to specifically whitelist URL-accessible actions now. Used in "Insert Link" form in HtmlEditorField. Regression from 1edf45fbedd1431f7b0105403b628deda2b61bdc --- forms/Form.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/forms/Form.php b/forms/Form.php index 6f692da8e..4dd0f84f2 100644 --- a/forms/Form.php +++ b/forms/Form.php @@ -149,7 +149,11 @@ class Form extends RequestHandler { */ protected $attributes = array(); - private static $allowed_actions = array('handleField', 'httpSubmission'); + private static $allowed_actions = array( + 'handleField', + 'httpSubmission', + 'forTemplate', + ); /** * Create a new form, with the given fields an action buttons. @@ -210,7 +214,7 @@ class Form extends RequestHandler { 'GET ' => 'httpSubmission', 'HEAD ' => 'httpSubmission', ); - + /** * Set up current form errors in session to * the current form if appropriate. From e55be5078337126fc6c6937c24f992fd1015eba7 Mon Sep 17 00:00:00 2001 From: Simon Welsh Date: Wed, 26 Jun 2013 15:49:00 +1200 Subject: [PATCH 35/54] FIX: ConfigStaticManifest not handling multipart namespaces Fixes #2126 --- core/manifest/ConfigStaticManifest.php | 26 ++++++++++++++----- .../manifest/ConfigStaticManifestTest.php | 18 ++++++++++++- .../ConfigStaticManifestTestNamespace.php | 10 +++++++ 3 files changed, 46 insertions(+), 8 deletions(-) create mode 100644 tests/core/manifest/ConfigStaticManifestTest/ConfigStaticManifestTestNamespace.php diff --git a/core/manifest/ConfigStaticManifest.php b/core/manifest/ConfigStaticManifest.php index a972e88c4..5dddd9266 100644 --- a/core/manifest/ConfigStaticManifest.php +++ b/core/manifest/ConfigStaticManifest.php @@ -78,7 +78,8 @@ class SS_ConfigStaticManifest { $static = $this->statics[$class][$name]; if ($static['access'] != T_PRIVATE) { - Deprecation::notice('3.2.0', "Config static $class::\$$name must be marked as private", Deprecation::SCOPE_GLOBAL); + Deprecation::notice('3.2.0', "Config static $class::\$$name must be marked as private", + Deprecation::SCOPE_GLOBAL); // Don't warn more than once per static $this->statics[$class][$name]['access'] = T_PRIVATE; } @@ -211,13 +212,23 @@ class SS_ConfigStaticManifest_Parser { $class = $next[1]; } else if($type == T_NAMESPACE) { - $next = $this->next(); + $namespace = ''; + while(true) { + $next = $this->next(); - if($next[0] != T_STRING) { - user_error("Couldn\'t parse {$this->path} when building config static manifest", E_USER_ERROR); + if($next == ';') { + break; + } elseif($next[0] == T_NS_SEPARATOR) { + $namespace .= $next[1]; + $next = $this->next(); + } + + if($next[0] != T_STRING) { + user_error("Couldn\'t parse {$this->path} when building config static manifest", E_USER_ERROR); + } + + $namespace .= $next[1]; } - - $namespace = $next[1]; } else if($type == '{' || $type == T_CURLY_OPEN || $type == T_DOLLAR_OPEN_CURLY_BRACES){ $depth += 1; @@ -288,7 +299,8 @@ class SS_ConfigStaticManifest_Parser { $depth -= 1; } - // Parse out the assignment side of a static declaration, ending on either a ';' or a ',' outside an array + // Parse out the assignment side of a static declaration, + // ending on either a ';' or a ',' outside an array if($type == T_WHITESPACE) { $value .= ' '; } diff --git a/tests/core/manifest/ConfigStaticManifestTest.php b/tests/core/manifest/ConfigStaticManifestTest.php index cb87841b5..87d1103db 100644 --- a/tests/core/manifest/ConfigStaticManifestTest.php +++ b/tests/core/manifest/ConfigStaticManifestTest.php @@ -170,7 +170,8 @@ DOC; return; } - $parser = new SS_ConfigStaticManifest_Parser(__DIR__ . '/ConfigStaticManifestTest/ConfigStaticManifestTestMyObject.php'); + $parser = new SS_ConfigStaticManifest_Parser(__DIR__ . + '/ConfigStaticManifestTest/ConfigStaticManifestTestMyObject.php'); $parser->parse(); $statics = $parser->getStatics(); @@ -182,4 +183,19 @@ DOC; $this->assertEquals($expectedValue, $statics['ConfigStaticManifestTestMyObject']['db']['value']); } + + public function testParsingNamespacesclass() { + $parser = new SS_ConfigStaticManifest_Parser(__DIR__ . + '/ConfigStaticManifestTest/ConfigStaticManifestTestNamespace.php'); + $parser->parse(); + + $statics = $parser->getStatics(); + + $expectedValue = array( + 'Name' => 'Varchar', + 'Description' => 'Text', + ); + + $this->assertEquals($expectedValue, $statics['config\staticmanifest\NamespaceTest']['db']['value']); + } } diff --git a/tests/core/manifest/ConfigStaticManifestTest/ConfigStaticManifestTestNamespace.php b/tests/core/manifest/ConfigStaticManifestTest/ConfigStaticManifestTestNamespace.php new file mode 100644 index 000000000..a2128af98 --- /dev/null +++ b/tests/core/manifest/ConfigStaticManifestTest/ConfigStaticManifestTestNamespace.php @@ -0,0 +1,10 @@ + 'Varchar', + 'Description' => 'Text', + ); +} From 1d5ac5876bfc1a75ba28fc46b63d0a66cd7c3233 Mon Sep 17 00:00:00 2001 From: Simon Welsh Date: Thu, 27 Jun 2013 09:49:10 +1200 Subject: [PATCH 36/54] Only redirect on logout if we're not already redirecting --- security/Security.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/security/Security.php b/security/Security.php index 2a4145945..498514d0b 100644 --- a/security/Security.php +++ b/security/Security.php @@ -346,7 +346,9 @@ class Security extends Controller { $member = Member::currentUser(); if($member) $member->logOut(); - if($redirect) $this->redirectBack(); + if($redirect && (!$this->response || !$this->response->isFinished())) { + $this->redirectBack(); + } } From 03aa9e4b41c64e690e6ab5a4d0e06a0f230afe09 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Fri, 28 Jun 2013 11:25:14 +1200 Subject: [PATCH 37/54] FIX ConfigManifest caching to not use existing cache from wrong $base --- core/manifest/ConfigManifest.php | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/core/manifest/ConfigManifest.php b/core/manifest/ConfigManifest.php index cf1d13008..b8cffb2c2 100644 --- a/core/manifest/ConfigManifest.php +++ b/core/manifest/ConfigManifest.php @@ -64,6 +64,7 @@ class SS_ConfigManifest { */ public function __construct($base, $includeTests = false, $forceRegen = false ) { $this->base = $base; + $this->key = sha1($base).'_'; // Get the Zend Cache to load/store cache into $this->cache = SS_Cache::factory('SS_Configuration', 'Core', array( @@ -74,14 +75,14 @@ class SS_ConfigManifest { // Unless we're forcing regen, try loading from cache if (!$forceRegen) { // The PHP config sources are always needed - $this->phpConfigSources = $this->cache->load('php_config_sources'); + $this->phpConfigSources = $this->cache->load($this->key.'php_config_sources'); // Get the variant key spec - $this->variantKeySpec = $this->cache->load('variant_key_spec'); + $this->variantKeySpec = $this->cache->load($this->key.'variant_key_spec'); // Try getting the pre-filtered & merged config for this variant - if (!($this->yamlConfig = $this->cache->load('yaml_config_'.$this->variantKey()))) { + if (!($this->yamlConfig = $this->cache->load($this->key.'yaml_config_'.$this->variantKey()))) { // Otherwise, if we do have the yaml config fragments (and we should since we have a variant key spec) // work out the config for this variant - if ($this->yamlConfigFragments = $this->cache->load('yaml_config_fragments')) { + if ($this->yamlConfigFragments = $this->cache->load($this->key.'yaml_config_fragments')) { $this->buildYamlConfigVariant(); } } @@ -165,9 +166,9 @@ class SS_ConfigManifest { $this->buildVariantKeySpec(); if ($cache) { - $this->cache->save($this->phpConfigSources, 'php_config_sources'); - $this->cache->save($this->yamlConfigFragments, 'yaml_config_fragments'); - $this->cache->save($this->variantKeySpec, 'variant_key_spec'); + $this->cache->save($this->phpConfigSources, $this->key.'php_config_sources'); + $this->cache->save($this->yamlConfigFragments, $this->key.'yaml_config_fragments'); + $this->cache->save($this->variantKeySpec, $this->key.'variant_key_spec'); } } @@ -493,7 +494,7 @@ class SS_ConfigManifest { } if ($cache) { - $this->cache->save($this->yamlConfig, 'yaml_config_'.$this->variantKey()); + $this->cache->save($this->yamlConfig, $this->key.'yaml_config_'.$this->variantKey()); } } From a9f150126c73cf9eefbe119e140692e3ea6ff1e7 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Fri, 28 Jun 2013 10:07:57 +0200 Subject: [PATCH 38/54] Fix CMSBatchActionHandler::$allowed_actions Regression from earlier API change to deny actions unless specified --- admin/code/CMSBatchActionHandler.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/admin/code/CMSBatchActionHandler.php b/admin/code/CMSBatchActionHandler.php index 0d61fb8d7..d3229e694 100644 --- a/admin/code/CMSBatchActionHandler.php +++ b/admin/code/CMSBatchActionHandler.php @@ -16,6 +16,12 @@ class CMSBatchActionHandler extends RequestHandler { '$BatchAction/confirmation' => 'handleConfirmation', '$BatchAction' => 'handleBatchAction', ); + + private static $allowed_actions = array( + 'handleBatchAction', + 'handleApplicablePages', + 'handleConfirmation', + ); protected $parentController; From a6c3d1e26939e4e3f57193f86285a0cae2659c75 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Fri, 28 Jun 2013 12:21:00 +0200 Subject: [PATCH 39/54] Flag "insert image" behat test as @assets Required in order to run them remotely, which currently doesn't support file upload through Selenium --- tests/behat/features/insert-an-image.feature | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/behat/features/insert-an-image.feature b/tests/behat/features/insert-an-image.feature index 8b5614eb9..c517579a0 100644 --- a/tests/behat/features/insert-an-image.feature +++ b/tests/behat/features/insert-an-image.feature @@ -1,3 +1,4 @@ +@assets Feature: Insert an image into a page As a cms author I want to insert an image into a page From e74c002647e6302618c880ab653bd48894692ec5 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Fri, 28 Jun 2013 13:46:32 +1200 Subject: [PATCH 40/54] FIX Only and Except rules in Configs not working --- core/manifest/ConfigManifest.php | 64 +++++++++---- tests/core/manifest/ConfigManifestTest.php | 94 +++++++++++++++++++ .../mysite/_config/classrules.yml | 28 ++++++ .../mysite/_config/constantdefinedrules.yml | 28 ++++++ .../mysite/_config/envvarsetrules.yml | 28 ++++++ .../mysite/_config/modulerules.yml | 28 ++++++ .../mysite/_config/variablerules.yml | 70 ++++++++++++++ 7 files changed, 320 insertions(+), 20 deletions(-) create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/classrules.yml create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/constantdefinedrules.yml create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/envvarsetrules.yml create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/modulerules.yml create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/variablerules.yml diff --git a/core/manifest/ConfigManifest.php b/core/manifest/ConfigManifest.php index b8cffb2c2..332f838c7 100644 --- a/core/manifest/ConfigManifest.php +++ b/core/manifest/ConfigManifest.php @@ -396,10 +396,17 @@ class SS_ConfigManifest { $matchingFragments = array(); foreach ($this->yamlConfigFragments as $i => $fragment) { - $failsonly = isset($fragment['only']) && !$this->matchesPrefilterVariantRules($fragment['only']); - $matchesexcept = isset($fragment['except']) && $this->matchesPrefilterVariantRules($fragment['except']); + $matches = true; - if (!$failsonly && !$matchesexcept) $matchingFragments[] = $fragment; + if (isset($fragment['only'])) { + $matches = $matches && ($this->matchesPrefilterVariantRules($fragment['only']) !== false); + } + + if (isset($fragment['except'])) { + $matches = $matches && ($this->matchesPrefilterVariantRules($fragment['except']) !== true); + } + + if ($matches) $matchingFragments[] = $fragment; } $this->yamlConfigFragments = $matchingFragments; @@ -414,22 +421,26 @@ class SS_ConfigManifest { * which values means accept or reject a fragment */ public function matchesPrefilterVariantRules($rules) { + $matches = "undefined"; // Needs to be truthy, but not true + foreach ($rules as $k => $v) { switch (strtolower($k)) { case 'classexists': - if (!ClassInfo::exists($v)) return false; + $matches = $matches && ClassInfo::exists($v); break; case 'moduleexists': - if (!$this->moduleExists($v)) return false; + $matches = $matches && $this->moduleExists($v); break; default: // NOP } + + if ($matches === false) return $matches; } - return true; + return $matches; } /** @@ -487,10 +498,17 @@ class SS_ConfigManifest { $this->yamlConfig = array(); foreach ($this->yamlConfigFragments as $i => $fragment) { - $failsonly = isset($fragment['only']) && !$this->matchesVariantRules($fragment['only']); - $matchesexcept = isset($fragment['except']) && $this->matchesVariantRules($fragment['except']); + $matches = true; - if (!$failsonly && !$matchesexcept) $this->mergeInYamlFragment($this->yamlConfig, $fragment['fragment']); + if (isset($fragment['only'])) { + $matches = $matches && ($this->matchesVariantRules($fragment['only']) !== false); + } + + if (isset($fragment['except'])) { + $matches = $matches && ($this->matchesVariantRules($fragment['except']) !== true); + } + + if ($matches) $this->mergeInYamlFragment($this->yamlConfig, $fragment['fragment']); } if ($cache) { @@ -502,6 +520,8 @@ class SS_ConfigManifest { * Returns false if the non-prefilterable parts of the rule aren't met, and true if they are */ public function matchesVariantRules($rules) { + $matches = "undefined"; // Needs to be truthy, but not true + foreach ($rules as $k => $v) { switch (strtolower($k)) { case 'classexists': @@ -511,13 +531,13 @@ class SS_ConfigManifest { case 'environment': switch (strtolower($v)) { case 'live': - if (!Director::isLive()) return false; + $matches = $matches && Director::isLive(); break; case 'test': - if (!Director::isTest()) return false; + $matches = $matches && Director::isTest(); break; case 'dev': - if (!Director::isDev()) return false; + $matches = $matches && Director::isDev(); break; default: user_error('Unknown environment '.$v.' in config fragment', E_USER_ERROR); @@ -525,21 +545,25 @@ class SS_ConfigManifest { break; case 'envvarset': - if (isset($_ENV[$k])) break; - return false; + $matches = $matches && isset($_ENV[$v]); + break; case 'constantdefined': - if (defined($k)) break; - return false; + $matches = $matches && defined($v); + break; default: - if (isset($_ENV[$k]) && $_ENV[$k] == $v) break; - if (defined($k) && constant($k) == $v) break; - return false; + $matches = $matches && ( + (isset($_ENV[$k]) && $_ENV[$k] == $v) || + (defined($k) && constant($k) == $v) + ); + break; } + + if ($matches === false) return $matches; } - return true; + return $matches; } /** diff --git a/tests/core/manifest/ConfigManifestTest.php b/tests/core/manifest/ConfigManifestTest.php index 85800ad0e..1250adbdc 100644 --- a/tests/core/manifest/ConfigManifestTest.php +++ b/tests/core/manifest/ConfigManifestTest.php @@ -8,6 +8,100 @@ class ConfigManifestTest_ConfigManifestAccess extends SS_ConfigManifest { class ConfigManifestTest extends SapphireTest { + protected function getConfigFixture() { + $manifest = new SS_ConfigManifest(dirname(__FILE__).'/fixtures/configmanifest', true, true); + return $manifest->yamlConfig; + } + + public function testClassRules() { + $config = $this->getConfigFixture(); + + $this->assertEquals( + 'Yes', @$config['ConfigManifestTest']['Class']['DirectorExists'], + 'Only rule correctly detects existing class' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['Class']['NoSuchClassExists'], + 'Except rule correctly detects missing class' + ); + } + + public function testModuleRules() { + $config = $this->getConfigFixture(); + + $this->assertEquals( + 'Yes', @$config['ConfigManifestTest']['Module']['MysiteExists'], + 'Only rule correctly detects existing module' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['Module']['NoSuchModuleExists'], + 'Except rule correctly detects missing module' + ); + } + + public function testEnvVarSetRules() { + $_ENV['EnvVarSet_Foo'] = 1; + $config = $this->getConfigFixture(); + + $this->assertEquals( + 'Yes', @$config['ConfigManifestTest']['EnvVarSet']['FooSet'], + 'Only rule correctly detects set environment variable' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['EnvVarSet']['BarSet'], + 'Except rule correctly detects unset environment variable' + ); + } + + public function testConstantDefinedRules() { + define('ConstantDefined_Foo', 1); + $config = $this->getConfigFixture(); + + $this->assertEquals( + 'Yes', @$config['ConfigManifestTest']['ConstantDefined']['FooDefined'], + 'Only rule correctly detects defined constant' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['ConstantDefined']['BarDefined'], + 'Except rule correctly detects undefined constant' + ); + } + + public function testEnvOrConstantMatchesValueRules() { + $_ENV['EnvOrConstantMatchesValue_Foo'] = 'Foo'; + define('EnvOrConstantMatchesValue_Bar', 'Bar'); + $config = $this->getConfigFixture(); + + $this->assertEquals( + 'Yes', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['FooIsFoo'], + 'Only rule correctly detects environment variable matches specified value' + ); + + $this->assertEquals( + 'Yes', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['BarIsBar'], + 'Only rule correctly detects constant matches specified value' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['FooIsQux'], + 'Except rule correctly detects environment variable that doesn\'t match specified value' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['BarIsQux'], + 'Except rule correctly detects environment variable that doesn\'t match specified value' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['BazIsBaz'], + 'Except rule correctly detects undefined variable' + ); + } + public function testRelativeOrder() { $accessor = new ConfigManifestTest_ConfigManifestAccess(BASE_PATH, true, false); diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/classrules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/classrules.yml new file mode 100644 index 000000000..eaadf2c7a --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/classrules.yml @@ -0,0 +1,28 @@ +--- +Only: + ClassExists: Director +--- +ConfigManifestTest: + Class: + DirectorExists: Yes +--- +Only: + ClassExists: NoSuchClass +--- +ConfigManifestTest: + Class: + NoSuchClassExists: Yes +--- +Except: + ClassExists: Director +--- +ConfigManifestTest: + Class: + DirectorExists: No +--- +Except: + ClassExists: NoSuchClass +--- +ConfigManifestTest: + Class: + NoSuchClassExists: No diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/constantdefinedrules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/constantdefinedrules.yml new file mode 100644 index 000000000..2833cb592 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/constantdefinedrules.yml @@ -0,0 +1,28 @@ +--- +Only: + ConstantDefined: ConstantDefined_Foo +--- +ConfigManifestTest: + ConstantDefined: + FooDefined: Yes +--- +Only: + ConstantDefined: ConstantDefined_Bar +--- +ConfigManifestTest: + ConstantDefined: + BarDefined: Yes +--- +Except: + ConstantDefined: ConstantDefined_Foo +--- +ConfigManifestTest: + ConstantDefined: + FooDefined: No +--- +Except: + ConstantDefined: ConstantDefined_Bar +--- +ConfigManifestTest: + ConstantDefined: + BarDefined: No diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/envvarsetrules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/envvarsetrules.yml new file mode 100644 index 000000000..45368aa00 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/envvarsetrules.yml @@ -0,0 +1,28 @@ +--- +Only: + EnvVarSet: EnvVarSet_Foo +--- +ConfigManifestTest: + EnvVarSet: + FooSet: Yes +--- +Only: + EnvVarSet: EnvVarSet_Bar +--- +ConfigManifestTest: + EnvVarSet: + BarSet: Yes +--- +Except: + EnvVarSet: EnvVarSet_Foo +--- +ConfigManifestTest: + EnvVarSet: + FooSet: No +--- +Except: + EnvVarSet: EnvVarSet_Bar +--- +ConfigManifestTest: + EnvVarSet: + BarSet: No diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/modulerules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/modulerules.yml new file mode 100644 index 000000000..1cb9232f6 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/modulerules.yml @@ -0,0 +1,28 @@ +--- +Only: + ModuleExists: mysite +--- +ConfigManifestTest: + Module: + MysiteExists: Yes +--- +Only: + ModuleExists: nosuchmodule +--- +ConfigManifestTest: + Module: + NoSuchModuleExists: Yes +--- +Except: + ModuleExists: mysite +--- +ConfigManifestTest: + Module: + MysiteExists: No +--- +Except: + ModuleExists: nosuchmodule +--- +ConfigManifestTest: + Module: + NoSuchModuleExists: No diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/variablerules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/variablerules.yml new file mode 100644 index 000000000..ebb7c61a1 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/variablerules.yml @@ -0,0 +1,70 @@ +--- +Only: + EnvOrConstantMatchesValue_Foo: Foo +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + FooIsFoo: Yes +--- +Only: + EnvOrConstantMatchesValue_Foo: Qux +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + FooIsQux: Yes +--- +Only: + EnvOrConstantMatchesValue_Bar: Bar +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + BarIsBar: Yes +--- +Only: + EnvOrConstantMatchesValue_Bar: Qux +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + BarIsQux: Yes +--- +Only: + EnvOrConstantMatchesValue_Baz: Baz +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + BazIsBaz: Yes +--- +Except: + EnvOrConstantMatchesValue_Foo: Foo +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + FooIsFoo: No +--- +Except: + EnvOrConstantMatchesValue_Foo: Qux +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + FooIsQux: No +--- +Except: + EnvOrConstantMatchesValue_Bar: Bar +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + BarIsBar: No +--- +Except: + EnvOrConstantMatchesValue_Bar: Qux +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + BarIsQux: No +--- +Except: + EnvOrConstantMatchesValue_Baz: Baz +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + BazIsBaz: No From 5484283a25d666fa1fb658da1a2bcfe6db3b8730 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Mon, 1 Jul 2013 15:25:21 +1200 Subject: [PATCH 41/54] FIX changing environment in config.php changes matched yaml rules --- core/Config.php | 13 ++- core/manifest/ConfigManifest.php | 94 ++++++++++++++++--- tests/core/manifest/ConfigManifestTest.php | 79 ++++++++++++---- .../mysite/_config/envrules.yml | 42 +++++++++ .../mysite/_config.php | 4 + .../mysite/_config/environment.yml | 18 ++++ 6 files changed, 215 insertions(+), 35 deletions(-) create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/envrules.yml create mode 100644 tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config.php create mode 100644 tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config/environment.yml diff --git a/core/Config.php b/core/Config.php index 10c00ed3c..752e73c27 100644 --- a/core/Config.php +++ b/core/Config.php @@ -295,8 +295,12 @@ class Config { * instead, the last manifest to be added always wins */ public function pushConfigYamlManifest(SS_ConfigManifest $manifest) { - array_unshift($this->manifests, $manifest->yamlConfig); + array_unshift($this->manifests, $manifest); + + // Now that we've got another yaml config manifest we need to clean the cache $this->cache->clean(); + // We also need to clean the cache if the manifest's calculated config values change + $manifest->registerChangeCallback(array($this->cache, 'clean')); // @todo: Do anything with these. They're for caching after config.php has executed $this->collectConfigPHPSettings = true; @@ -479,10 +483,13 @@ class Config { } } + $value = $nothing = null; + // Then the manifest values foreach($this->manifests as $manifest) { - if (isset($manifest[$class][$name])) { - self::merge_low_into_high($result, $manifest[$class][$name], $suppress); + $value = $manifest->get($class, $name, $nothing); + if ($value !== $nothing) { + self::merge_low_into_high($result, $value, $suppress); if ($result !== null && !is_array($result)) return $result; } } diff --git a/core/manifest/ConfigManifest.php b/core/manifest/ConfigManifest.php index 332f838c7..1398518aa 100644 --- a/core/manifest/ConfigManifest.php +++ b/core/manifest/ConfigManifest.php @@ -9,7 +9,16 @@ */ class SS_ConfigManifest { - /** + /** @var string - The base path used when building the manifest */ + protected $base; + + /** @var string - A string to prepend to all cache keys to ensure all keys are unique to just this $base */ + protected $key; + + /** @var bool - Whether `test` directories should be searched when searching for configuration */ + protected $includeTests; + + /** * All the values needed to be collected to determine the correct combination of fragements for * the current environment. * @var array @@ -34,6 +43,17 @@ class SS_ConfigManifest { */ public $yamlConfig = array(); + /** + * The variant key state as when yamlConfig was loaded + * @var string + */ + protected $yamlConfigVariantKey = null; + + /** + * @var [callback] A list of callbacks to be called whenever the content of yamlConfig changes + */ + protected $configChangeCallbacks = array(); + /** * A side-effect of collecting the _config fragments is the calculation of all module directories, since * the definition of a module is "a directory that contains either a _config.php file or a _config directory @@ -65,6 +85,7 @@ class SS_ConfigManifest { public function __construct($base, $includeTests = false, $forceRegen = false ) { $this->base = $base; $this->key = sha1($base).'_'; + $this->includeTests = $includeTests; // Get the Zend Cache to load/store cache into $this->cache = SS_Cache::factory('SS_Configuration', 'Core', array( @@ -78,21 +99,28 @@ class SS_ConfigManifest { $this->phpConfigSources = $this->cache->load($this->key.'php_config_sources'); // Get the variant key spec $this->variantKeySpec = $this->cache->load($this->key.'variant_key_spec'); - // Try getting the pre-filtered & merged config for this variant - if (!($this->yamlConfig = $this->cache->load($this->key.'yaml_config_'.$this->variantKey()))) { - // Otherwise, if we do have the yaml config fragments (and we should since we have a variant key spec) - // work out the config for this variant - if ($this->yamlConfigFragments = $this->cache->load($this->key.'yaml_config_fragments')) { - $this->buildYamlConfigVariant(); - } - } } - // If we don't have a config yet, we need to do a full regen to get it - if (!$this->yamlConfig) { + // If we don't have a variantKeySpec (because we're forcing regen, or it just wasn't in the cache), generate it + if (!$this->variantKeySpec) { $this->regenerate($includeTests); - $this->buildYamlConfigVariant(); } + + // At this point $this->variantKeySpec will always contain something valid, so we can build the variant + $this->buildYamlConfigVariant(); + } + + /** + * Register a callback to be called whenever the calculated merged config changes + * + * In some situations the merged config can change - for instance, code in _config.php can cause which Only + * and Except fragments match. Registering a callback with this function allows code to be called when + * this happens. + * + * @param callback $callback + */ + public function registerChangeCallback($callback) { + $this->configChangeCallbacks[] = $callback; } /** @@ -103,6 +131,22 @@ class SS_ConfigManifest { foreach ($this->phpConfigSources as $config) { require_once $config; } + + if ($this->variantKey() != $this->yamlConfigVariantKey) $this->buildYamlConfigVariant(); + } + + /** + * Gets the (merged) config value for the given class and config property name + * + * @param string $class - The class to get the config property value for + * @param string $name - The config property to get the value for + * @param any $default - What to return if no value was contained in any YAML file for the passed $class and $name + * @return any - The merged set of all values contained in all the YAML configuration files for the passed + * $class and $name, or $default if there are none + */ + public function get($class, $name, $default=null) { + if (isset($this->yamlConfig[$class][$name])) return $this->yamlConfig[$class][$name]; + else return $default; } /** @@ -493,9 +537,32 @@ class SS_ConfigManifest { /** * Calculates which yaml config fragments are applicable in this variant, and merge those all together into * the $this->yamlConfig propperty + * + * Checks cache and takes care of loading yamlConfigFragments if they aren't already present, but expects + * $variantKeySpec to already be set */ public function buildYamlConfigVariant($cache = true) { + // Only try loading from cache if we don't have the fragments already loaded, as there's no way to know if a + // given variant is stale compared to the complete set of fragments + if (!$this->yamlConfigFragments) { + // First try and just load the exact variant + if ($this->yamlConfig = $this->cache->load($this->key.'yaml_config_'.$this->variantKey())) { + $this->yamlConfigVariantKey = $this->variantKey(); + return; + } + // Otherwise try and load the fragments so we can build the variant + else { + $this->yamlConfigFragments = $this->cache->load($this->key.'yaml_config_fragments'); + } + } + + // If we still don't have any fragments we have to build them + if (!$this->yamlConfigFragments) { + $this->regenerate($this->includeTests, $cache); + } + $this->yamlConfig = array(); + $this->yamlConfigVariantKey = $this->variantKey(); foreach ($this->yamlConfigFragments as $i => $fragment) { $matches = true; @@ -514,6 +581,9 @@ class SS_ConfigManifest { if ($cache) { $this->cache->save($this->yamlConfig, $this->key.'yaml_config_'.$this->variantKey()); } + + // Since yamlConfig has changed, call any callbacks that are interested + foreach ($this->configChangeCallbacks as $callback) call_user_func($callback); } /** diff --git a/tests/core/manifest/ConfigManifestTest.php b/tests/core/manifest/ConfigManifestTest.php index 1250adbdc..f11c022d7 100644 --- a/tests/core/manifest/ConfigManifestTest.php +++ b/tests/core/manifest/ConfigManifestTest.php @@ -8,65 +8,65 @@ class ConfigManifestTest_ConfigManifestAccess extends SS_ConfigManifest { class ConfigManifestTest extends SapphireTest { - protected function getConfigFixture() { + protected function getConfigFixtureValue($name) { $manifest = new SS_ConfigManifest(dirname(__FILE__).'/fixtures/configmanifest', true, true); - return $manifest->yamlConfig; + return $manifest->get('ConfigManifestTest', $name); } public function testClassRules() { - $config = $this->getConfigFixture(); + $config = $this->getConfigFixtureValue('Class'); $this->assertEquals( - 'Yes', @$config['ConfigManifestTest']['Class']['DirectorExists'], + 'Yes', @$config['DirectorExists'], 'Only rule correctly detects existing class' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['Class']['NoSuchClassExists'], + 'No', @$config['NoSuchClassExists'], 'Except rule correctly detects missing class' ); } public function testModuleRules() { - $config = $this->getConfigFixture(); + $config = $this->getConfigFixtureValue('Module'); $this->assertEquals( - 'Yes', @$config['ConfigManifestTest']['Module']['MysiteExists'], + 'Yes', @$config['MysiteExists'], 'Only rule correctly detects existing module' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['Module']['NoSuchModuleExists'], + 'No', @$config['NoSuchModuleExists'], 'Except rule correctly detects missing module' ); } public function testEnvVarSetRules() { $_ENV['EnvVarSet_Foo'] = 1; - $config = $this->getConfigFixture(); + $config = $this->getConfigFixtureValue('EnvVarSet'); $this->assertEquals( - 'Yes', @$config['ConfigManifestTest']['EnvVarSet']['FooSet'], + 'Yes', @$config['FooSet'], 'Only rule correctly detects set environment variable' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['EnvVarSet']['BarSet'], + 'No', @$config['BarSet'], 'Except rule correctly detects unset environment variable' ); } public function testConstantDefinedRules() { define('ConstantDefined_Foo', 1); - $config = $this->getConfigFixture(); + $config = $this->getConfigFixtureValue('ConstantDefined'); $this->assertEquals( - 'Yes', @$config['ConfigManifestTest']['ConstantDefined']['FooDefined'], + 'Yes', @$config['FooDefined'], 'Only rule correctly detects defined constant' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['ConstantDefined']['BarDefined'], + 'No', @$config['BarDefined'], 'Except rule correctly detects undefined constant' ); } @@ -74,34 +74,73 @@ class ConfigManifestTest extends SapphireTest { public function testEnvOrConstantMatchesValueRules() { $_ENV['EnvOrConstantMatchesValue_Foo'] = 'Foo'; define('EnvOrConstantMatchesValue_Bar', 'Bar'); - $config = $this->getConfigFixture(); + $config = $this->getConfigFixtureValue('EnvOrConstantMatchesValue'); $this->assertEquals( - 'Yes', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['FooIsFoo'], + 'Yes', @$config['FooIsFoo'], 'Only rule correctly detects environment variable matches specified value' ); $this->assertEquals( - 'Yes', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['BarIsBar'], + 'Yes', @$config['BarIsBar'], 'Only rule correctly detects constant matches specified value' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['FooIsQux'], + 'No', @$config['FooIsQux'], 'Except rule correctly detects environment variable that doesn\'t match specified value' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['BarIsQux'], + 'No', @$config['BarIsQux'], 'Except rule correctly detects environment variable that doesn\'t match specified value' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['BazIsBaz'], + 'No', @$config['BazIsBaz'], 'Except rule correctly detects undefined variable' ); } + public function testEnvironmentRules() { + foreach (array('dev', 'test', 'live') as $env) { + Config::inst()->nest(); + + Config::inst()->update('Director', 'environment_type', $env); + $config = $this->getConfigFixtureValue('Environment'); + + foreach (array('dev', 'test', 'live') as $check) { + $this->assertEquals( + $env == $check ? $check : 'not'.$check, @$config[ucfirst($check).'Environment'], + 'Only & except rules correctly detect environment' + ); + } + + Config::inst()->unnest(); + } + } + + public function testDynamicEnvironmentRules() { + Config::inst()->nest(); + + // First, make sure environment_type is live + Config::inst()->update('Director', 'environment_type', 'live'); + $this->assertEquals('live', Config::inst()->get('Director', 'environment_type')); + + // Then, load in a new manifest, which includes a _config.php that sets environment_type to dev + $manifest = new SS_ConfigManifest(dirname(__FILE__).'/fixtures/configmanifest_dynamicenv', true, true); + Config::inst()->pushConfigYamlManifest($manifest); + + // Make sure that stuck + $this->assertEquals('dev', Config::inst()->get('Director', 'environment_type')); + + // And that the dynamic rule was calculated correctly + $this->assertEquals('dev', Config::inst()->get('ConfigManifestTest', 'DynamicEnvironment')); + + Config::inst()->unnest(); + } + + public function testRelativeOrder() { $accessor = new ConfigManifestTest_ConfigManifestAccess(BASE_PATH, true, false); diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/envrules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/envrules.yml new file mode 100644 index 000000000..6573ac565 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/envrules.yml @@ -0,0 +1,42 @@ +--- +Only: + Environment: live +--- +ConfigManifestTest: + Environment: + LiveEnvironment: live +--- +Only: + Environment: dev +--- +ConfigManifestTest: + Environment: + DevEnvironment: dev +--- +Only: + Environment: test +--- +ConfigManifestTest: + Environment: + TestEnvironment: test +--- +Except: + Environment: live +--- +ConfigManifestTest: + Environment: + LiveEnvironment: notlive +--- +Except: + Environment: dev +--- +ConfigManifestTest: + Environment: + DevEnvironment: notdev +--- +Except: + Environment: test +--- +ConfigManifestTest: + Environment: + TestEnvironment: nottest diff --git a/tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config.php b/tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config.php new file mode 100644 index 000000000..337294dec --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config.php @@ -0,0 +1,4 @@ +update('Director', 'environment_type', 'dev'); diff --git a/tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config/environment.yml b/tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config/environment.yml new file mode 100644 index 000000000..fb5e190e3 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config/environment.yml @@ -0,0 +1,18 @@ +--- +Only: + Environment: live +--- +ConfigManifestTest: + DynamicEnvironment: live +--- +Only: + Environment: dev +--- +ConfigManifestTest: + DynamicEnvironment: dev +--- +Only: + Environment: test +--- +ConfigManifestTest: + DynamicEnvironment: test From df218d76dae5f8e12b856200e93c97800612d6a8 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Tue, 2 Jul 2013 13:54:06 +1200 Subject: [PATCH 42/54] Clarify how Only and Except rules combine --- docs/en/topics/configuration.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/en/topics/configuration.md b/docs/en/topics/configuration.md index 95b77fc09..580e3167f 100644 --- a/docs/en/topics/configuration.md +++ b/docs/en/topics/configuration.md @@ -227,8 +227,8 @@ To accommodate this, value sections can be filtered to only be used when either current environment. To achieve this you add a key to the related header section, either "Only" when the value section should be included -only when the rules contained match, or "Except" when the value section should be included except when the rules -contained match. +only when all the rules contained match, or "Except" when the value section should be included except when all of the +rules contained match. You then list any of the following rules as sub-keys, with informational values as either a single value or a list. @@ -256,6 +256,12 @@ For instance, to add a property to "foo" when a module exists, and "bar" otherwi property: 'bar' --- +Note than when you have more than one rule for a nested fragment, they're joined like + + FRAGMENT_INCLUDED = (ONLY && ONLY) && !(EXCEPT && EXCEPT) + +That is, the fragment will be included if all Only rules match, except if all Except rules match + ### The values The values section of YAML configuration files is quite simple - it is simply a nested key / value pair structure From f9ede95e5b901a89f2b0bc912eddfc8d5b10ac74 Mon Sep 17 00:00:00 2001 From: Mateusz Uzdowski Date: Tue, 2 Jul 2013 15:51:53 +1200 Subject: [PATCH 43/54] Add configuration system tests for Only and Except combinations. --- docs/en/topics/configuration.md | 5 +- tests/core/manifest/ConfigManifestTest.php | 37 +++++++++++++- .../mysite/_config/multiplerules.yml | 50 +++++++++++++++++++ 3 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/multiplerules.yml diff --git a/docs/en/topics/configuration.md b/docs/en/topics/configuration.md index 580e3167f..f0dc40073 100644 --- a/docs/en/topics/configuration.md +++ b/docs/en/topics/configuration.md @@ -260,7 +260,10 @@ Note than when you have more than one rule for a nested fragment, they're joined FRAGMENT_INCLUDED = (ONLY && ONLY) && !(EXCEPT && EXCEPT) -That is, the fragment will be included if all Only rules match, except if all Except rules match +That is, the fragment will be included if all Only rules match, except if all Except rules match. + +Also, due to YAML limitations, having multiple conditions of the same kind (say, two `EnvVarSet` in one "Only" block) +will result in only the latter coming through. ### The values diff --git a/tests/core/manifest/ConfigManifestTest.php b/tests/core/manifest/ConfigManifestTest.php index f11c022d7..27a2da60f 100644 --- a/tests/core/manifest/ConfigManifestTest.php +++ b/tests/core/manifest/ConfigManifestTest.php @@ -140,6 +140,41 @@ class ConfigManifestTest extends SapphireTest { Config::inst()->unnest(); } + public function testMultipleRules() { + $_ENV['MultilpleRules_EnvVariableSet'] = 1; + define('MultilpleRules_DefinedConstant', 'defined'); + $config = $this->getConfigFixtureValue('MultipleRules'); + + $this->assertFalse( + isset($config['TwoOnlyFail']), + 'Fragment is not included if one of the Only rules fails.' + ); + + $this->assertTrue( + isset($config['TwoOnlySucceed']), + 'Fragment is included if both Only rules succeed.' + ); + + $this->assertTrue( + isset($config['TwoExceptSucceed']), + 'Fragment is included if one of the Except rules matches.' + ); + + $this->assertFalse( + isset($config['TwoExceptFail']), + 'Fragment is not included if both of the Except rules fail.' + ); + + $this->assertFalse( + isset($config['TwoBlocksFail']), + 'Fragment is not included if one block fails.' + ); + + $this->assertTrue( + isset($config['TwoBlocksSucceed']), + 'Fragment is included if both blocks succeed.' + ); + } public function testRelativeOrder() { $accessor = new ConfigManifestTest_ConfigManifestAccess(BASE_PATH, true, false); @@ -221,4 +256,4 @@ class ConfigManifestTest extends SapphireTest { ), 'after'); } -} \ No newline at end of file +} diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/multiplerules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/multiplerules.yml new file mode 100644 index 000000000..459e4e635 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/multiplerules.yml @@ -0,0 +1,50 @@ +--- +Only: + ConstantDefined: MultilpleRules_UndefinedConstant + EnvVarSet: MultilpleRules_EnvVariableSet +--- +ConfigManifestTest: + MultipleRules: + TwoOnlyFail: "not included - one of the onlies fails" +--- +Only: + ConstantDefined: MultilpleRules_DefinedConstant + EnvVarSet: MultilpleRules_EnvVariableSet +--- +ConfigManifestTest: + MultipleRules: + TwoOnlySucceed: "included - both onlies succeed" +--- +Except: + ConstantDefined: MultilpleRules_UndefinedConstant + EnvVarSet: MultilpleRules_EnvVariableSet +--- +ConfigManifestTest: + MultipleRules: + TwoExceptSucceed: "included - one of the excepts succeeds" +--- +Except: + ConstantDefined: MultilpleRules_DefinedConstant + EnvVarSet: MultilpleRules_EnvVariableSet +--- +ConfigManifestTest: + MultipleRules: + TwoExceptFail: "not included - both excepts fail" +--- +Except: + EnvVarSet: MultilpleRules_EnvVariableSet +Only: + EnvVarSet: MultilpleRules_EnvVariableSet +--- +ConfigManifestTest: + MultipleRules: + TwoBlocksFail: "not included - one block fails" +--- +Except: + ConstantDefined: MultilpleRules_UndefinedConstant +Only: + EnvVarSet: MultilpleRules_EnvVariableSet +--- +ConfigManifestTest: + MultipleRules: + TwoBlocksSucceed: "included - both blocks succeed" From 50e9eee2e9dcfc5c14608f5e72adf06592b74f7b Mon Sep 17 00:00:00 2001 From: Jeremy Thomerson Date: Tue, 2 Jul 2013 13:09:05 +0000 Subject: [PATCH 44/54] FIX #2174: SearchFilter needs casting helper for DataObject base fields Commit 964b3f2 fixed an issue where dbObject was returning casting helpers for fields that were not actually DB objects, but had something in $casting config. However, because dbObject was no longer calling DataObject->castingHelper, this exposed a bug that the underlying function db($fieldName) was not returning field specs for the base fields that are created by SS automatically on all DataObjects (i.e. Created, LastEdited, etc). This commit fixes the underlying issue that DataObject->db($fieldName) should return the field specs for *all* DB fields like its documentation says it will, including those base fields that are automatically created and do not appear in $db. --- model/DataObject.php | 41 ++++++++++++++++++++------------ search/filters/SearchFilter.php | 2 +- tests/model/DataListTest.php | 42 ++++++++++++++++++++++++++++++++- tests/model/DataObjectTest.php | 15 +++++++++++- 4 files changed, 82 insertions(+), 18 deletions(-) diff --git a/model/DataObject.php b/model/DataObject.php index a0efd74c5..6127e5fb4 100644 --- a/model/DataObject.php +++ b/model/DataObject.php @@ -156,6 +156,14 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity protected static $_cache_custom_database_fields = array(); protected static $_cache_field_labels = array(); + // base fields which are not defined in static $db + private static $fixed_fields = array( + 'ID' => 'Int', + 'ClassName' => 'Enum', + 'LastEdited' => 'SS_Datetime', + 'Created' => 'SS_Datetime', + ); + /** * Non-static relationship cache, indexed by component name. */ @@ -223,6 +231,8 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity } return array_merge ( + // TODO: should this be using self::$fixed_fields? only difference is ID field + // and ClassName creates an Enum with all values array ( 'ClassName' => self::$classname_spec_cache[$class], 'Created' => 'SS_Datetime', @@ -1651,11 +1661,16 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity /** * Return all of the database fields defined in self::$db and all the parent classes. * Doesn't include any fields specified by self::$has_one. Use $this->has_one() to get these fields + * Also returns "base" fields like "Created", "LastEdited", et cetera. * * @param string $fieldName Limit the output to a specific field name * @return array The database fields */ public function db($fieldName = null) { + if ($fieldName && array_key_exists($fieldName, self::$fixed_fields)) { + return self::$fixed_fields[$fieldName]; + } + $classes = ClassInfo::ancestry($this); $good = false; $items = array(); @@ -1694,6 +1709,10 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity } } + if (!$fieldName) { + // trying to get all fields, so add the fixed fields to return value + $items = array_merge(self::$fixed_fields, $items); + } return $items; } @@ -2387,15 +2406,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity * @return boolean */ public function hasDatabaseField($field) { - // Add base fields which are not defined in static $db - static $fixedFields = array( - 'ID' => 'Int', - 'ClassName' => 'Enum', - 'LastEdited' => 'SS_Datetime', - 'Created' => 'SS_Datetime', - ); - - if(isset($fixedFields[$field])) return true; + if(isset(self::$fixed_fields[$field])) return true; return array_key_exists($field, $this->inheritedDatabaseFields()); } @@ -2658,7 +2669,12 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity // Special case for ID field } else if($fieldName == 'ID') { return new PrimaryKey($fieldName, $this); - + + // Special case for ClassName + } else if($fieldName == 'ClassName') { + $val = get_class($this); + return DBField::create_field('Varchar', $val, $fieldName, $this); + // General casting information for items in $db } else if($helper = $this->db($fieldName)) { $obj = Object::create_from_string($helper, $fieldName); @@ -2669,11 +2685,6 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity } else if(preg_match('/ID$/', $fieldName) && $this->has_one(substr($fieldName,0,-2))) { $val = $this->$fieldName; return DBField::create_field('ForeignKey', $val, $fieldName, $this); - - // Special case for ClassName - } else if($fieldName == 'ClassName') { - $val = get_class($this); - return DBField::create_field('Varchar', $val, $fieldName, $this); } } diff --git a/search/filters/SearchFilter.php b/search/filters/SearchFilter.php index 7a02b0e66..c647a3eab 100644 --- a/search/filters/SearchFilter.php +++ b/search/filters/SearchFilter.php @@ -300,4 +300,4 @@ abstract class SearchFilter extends Object { else return null; } -} \ No newline at end of file +} diff --git a/tests/model/DataListTest.php b/tests/model/DataListTest.php index e6e2b9941..456d48120 100644 --- a/tests/model/DataListTest.php +++ b/tests/model/DataListTest.php @@ -21,7 +21,47 @@ class DataListTest extends SapphireTest { 'DataObjectTest_TeamComment', 'DataObjectTest\NamespacedClass', ); - + + public function testFilterDataObjectByCreatedDate() { + // create an object to test with + $obj1 = new DataObjectTest_ValidatedObject(); + $obj1->Name = 'test obj 1'; + $obj1->write(); + $this->assertTrue($obj1->isInDB()); + + // reload the object from the database and reset its Created timestamp to a known value + $obj1 = DataObjectTest_ValidatedObject::get()->filter(array('Name' => 'test obj 1'))->first(); + $this->assertTrue(is_object($obj1)); + $this->assertEquals('test obj 1', $obj1->Name); + $obj1->Created = '2013-01-01 00:00:00'; + $obj1->write(); + + // reload the object again and make sure that our Created date was properly persisted + $obj1 = DataObjectTest_ValidatedObject::get()->filter(array('Name' => 'test obj 1'))->first(); + $this->assertTrue(is_object($obj1)); + $this->assertEquals('test obj 1', $obj1->Name); + $this->assertEquals('2013-01-01 00:00:00', $obj1->Created); + + // now save a second object to the DB with an automatically-set Created value + $obj2 = new DataObjectTest_ValidatedObject(); + $obj2->Name = 'test obj 2'; + $obj2->write(); + $this->assertTrue($obj2->isInDB()); + + // and a third object + $obj3 = new DataObjectTest_ValidatedObject(); + $obj3->Name = 'test obj 3'; + $obj3->write(); + $this->assertTrue($obj3->isInDB()); + + // now test the filtering based on Created timestamp + $list = DataObjectTest_ValidatedObject::get() + ->filter(array('Created:GreaterThan' => '2013-02-01 00:00:00')) + ->toArray(); + $this->assertEquals(2, count($list)); + + } + public function testSubtract(){ $comment1 = $this->objFromFixture('DataObjectTest_TeamComment', 'comment1'); $subtractList = DataObjectTest_TeamComment::get()->filter('ID', $comment1->ID); diff --git a/tests/model/DataObjectTest.php b/tests/model/DataObjectTest.php index 534bdbda1..fee16b636 100644 --- a/tests/model/DataObjectTest.php +++ b/tests/model/DataObjectTest.php @@ -18,7 +18,20 @@ class DataObjectTest extends SapphireTest { 'DataObjectTest_Player', 'DataObjectTest_TeamComment' ); - + + public function testValidObjectsForBaseFields() { + $obj = new DataObjectTest_ValidatedObject(); + + foreach (array('Created', 'LastEdited', 'ClassName', 'ID') as $field) { + $helper = $obj->dbObject($field); + $this->assertTrue( + ($helper instanceof DBField), + "for {$field} expected helper to be DBField, but was " . + (is_object($helper) ? get_class($helper) : "null") + ); + } + } + public function testDataIntegrityWhenTwoSubclassesHaveSameField() { // Save data into DataObjectTest_SubTeam.SubclassDatabaseField $obj = new DataObjectTest_SubTeam(); From d003c96c62a5d7e9e50deeb2d82efb3787e9a430 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 3 Jul 2013 10:15:46 +0200 Subject: [PATCH 45/54] Fixed HTMLEditorField extension call ("updateFieldsForOembed") --- forms/HtmlEditorField.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forms/HtmlEditorField.php b/forms/HtmlEditorField.php index 5ce926636..24902b76d 100644 --- a/forms/HtmlEditorField.php +++ b/forms/HtmlEditorField.php @@ -665,7 +665,7 @@ class HtmlEditorField_Toolbar extends RequestHandler { ), 'CaptionText'); } - $this->extend('updateFieldsForImage', $fields, $url, $file); + $this->extend('updateFieldsForOembed', $fields, $url, $file); return $fields; } From dacb2aa6381e2b126e8b0dcf261d7131beb38aa3 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Wed, 3 Jul 2013 20:06:49 +1200 Subject: [PATCH 46/54] FIX HtmlEditorField not re-checking sanitisation server side --- docs/en/changelogs/3.1.0.md | 1 + docs/en/topics/security.md | 9 + forms/HtmlEditorField.php | 13 +- forms/HtmlEditorSanitiser.php | 304 ++++++++++++++++++++++++ tests/forms/HtmlEditorSanitiserTest.php | 57 +++++ 5 files changed, 383 insertions(+), 1 deletion(-) create mode 100644 forms/HtmlEditorSanitiser.php create mode 100644 tests/forms/HtmlEditorSanitiserTest.php diff --git a/docs/en/changelogs/3.1.0.md b/docs/en/changelogs/3.1.0.md index f6c6e05e4..8795f02e5 100644 --- a/docs/en/changelogs/3.1.0.md +++ b/docs/en/changelogs/3.1.0.md @@ -35,6 +35,7 @@ * Optional integration with ImageMagick as a new image manipulation backend * Support for PHP 5.4's built-in webserver * Support for [Composer](http://getcomposer.org) dependency manager (also works with 3.0) + * Added support for filtering incoming HTML from TinyMCE (disabled by default, see [security](/topics/security)) ## Upgrading diff --git a/docs/en/topics/security.md b/docs/en/topics/security.md index 2d711ba80..acc9ba89a 100644 --- a/docs/en/topics/security.md +++ b/docs/en/topics/security.md @@ -272,6 +272,15 @@ Some rules of thumb: * Don't concatenate URLs in a template. It only works in extremely simple cases that usually contain bugs. * Use *Controller::join_links()* to concatenate URLs. It deals with query strings and other such edge cases. +### Filtering incoming HTML from TinyMCE + +In some cases you may be particularly concerned about which HTML elements are addable to Content via the CMS. +By default, although TinyMCE is configured to restrict some dangerous tags (such as `script` tags), this restriction +is not enforced server-side. A malicious user with write access to the CMS might create a specific request to avoid +these restrictions. + +To enable server side filtering using the same whitelisting controls as TinyMCE, set the +HtmlEditorField::$sanitise_server_side config property to true. ## Cross-Site Request Forgery (CSRF) diff --git a/forms/HtmlEditorField.php b/forms/HtmlEditorField.php index 5ce926636..31827834a 100644 --- a/forms/HtmlEditorField.php +++ b/forms/HtmlEditorField.php @@ -20,6 +20,12 @@ class HtmlEditorField extends TextareaField { */ private static $insert_width = 600; + /** + * @config + * @var bool Should we check the valid_elements (& extended_valid_elements) rules from HtmlEditorConfig server side? + */ + private static $sanitise_server_side = false; + protected $rows = 30; /** @@ -111,7 +117,12 @@ class HtmlEditorField extends TextareaField { $linkedFiles = array(); $htmlValue = Injector::inst()->create('HTMLValue', $this->value); - + + if($this->config()->sanitise_server_side) { + $santiser = Injector::inst()->create('HtmlEditorSanitiser', HtmlEditorConfig::get_active()); + $santiser->sanitise($htmlValue); + } + if(class_exists('SiteTree')) { // Populate link tracking for internal links & links to asset files. if($links = $htmlValue->getElementsByTagName('a')) foreach($links as $link) { diff --git a/forms/HtmlEditorSanitiser.php b/forms/HtmlEditorSanitiser.php new file mode 100644 index 000000000..98b78faf7 --- /dev/null +++ b/forms/HtmlEditorSanitiser.php @@ -0,0 +1,304 @@ + $rule hash for whitelist element rules where the element name isn't a pattern */ + protected $elements = array(); + /** @var [stdClass] - Sequential list of whitelist element rules where the element name is a pattern */ + protected $elementPatterns = array(); + + /** @var [stdClass] - The list of attributes that apply to all further whitelisted elements added */ + protected $globalAttributes = array(); + + /** + * Construct a sanitiser from a given HtmlEditorConfig + * + * Note that we build data structures from the current state of HtmlEditorConfig - later changes to + * the passed instance won't cause this instance to update it's whitelist + * + * @param HtmlEditorConfig $config + */ + public function __construct(HtmlEditorConfig $config) { + $valid = $config->getOption('valid_elements'); + if ($valid) $this->addValidElements($valid); + + $valid = $config->getOption('extended_valid_elements'); + if ($valid) $this->addValidElements($valid); + } + + /** + * Given a TinyMCE pattern (close to unix glob style), create a regex that does the match + * + * @param $str - The TinyMCE pattern + * @return string - The equivalent regex + */ + protected function patternToRegex($str) { + return '/^' . preg_replace('/([?+*])/', '.$1', $str) . '$/'; + } + + /** + * Given a valid_elements string, parse out the actual element and attribute rules and add to the + * internal whitelist + * + * Logic based heavily on javascript version from tiny_mce_src.js + * + * @param string $validElements - The valid_elements or extended_valid_elements string to add to the whitelist + */ + protected function addValidElements($validElements) { + $elementRuleRegExp = '/^([#+\-])?([^\[\/]+)(?:\/([^\[]+))?(?:\[([^\]]+)\])?$/'; + $attrRuleRegExp = '/^([!\-])?(\w+::\w+|[^=:<]+)?(?:([=:<])(.*))?$/'; + $hasPatternsRegExp = '/[*?+]/'; + + foreach(explode(',', $validElements) as $validElement) { + if(preg_match($elementRuleRegExp, $validElement, $matches)) { + + $prefix = @$matches[1]; + $elementName = @$matches[2]; + $outputName = @$matches[3]; + $attrData = @$matches[4]; + + // Create the new element + $element = new stdClass(); + $element->attributes = array(); + $element->attributePatterns = array(); + + $element->attributesRequired = array(); + $element->attributesDefault = array(); + $element->attributesForced = array(); + + foreach(array('#' => 'paddEmpty', '-' => 'removeEmpty') as $match => $means) { + $element->$means = ($prefix === $match); + } + + // Copy attributes from global rule into current rule + if($this->globalAttributes) { + $element->attributes = array_merge($element->attributes, $this->globalAttributes); + } + + // Attributes defined + if($attrData) { + foreach(explode('|', $attrData) as $attr) { + if(preg_match($attrRuleRegExp, $attr, $matches)) { + $attr = new stdClass(); + + $attrType = @$matches[1]; + $attrName = str_replace('::', ':', @$matches[2]); + $prefix = @$matches[3]; + $value = @$matches[4]; + + // Required + if($attrType === '!') { + $element->attributesRequired[] = $attrName; + $attr->required = true; + } + + // Denied from global + else if($attrType === '-') { + unset($element->attributes[$attrName]); + continue; + } + + // Default value + if($prefix) { + // Default value + if($prefix === '=') { + $element->attributesDefault[$attrName] = $value; + $attr->defaultValue = $value; + } + + // Forced value + else if($prefix === ':') { + $element->attributesForced[$attrName] = $value; + $attr->forcedValue = $value; + } + + // Required values + else if($prefix === '<') { + $attr->validValues = explode('?', $value); + } + } + + // Check for attribute patterns + if(preg_match($hasPatternsRegExp, $attrName)) { + $attr->pattern = $this->patternToRegex($attrName); + $element->attributePatterns[] = $attr; + } + else { + $element->attributes[$attrName] = $attr; + } + } + } + } + + // Global rule, store away these for later usage + if(!$this->globalAttributes && $elementName == '@') { + $this->globalAttributes = $element->attributes; + } + + // Handle substitute elements such as b/strong + if($outputName) { + $element->outputName = $elementName; + $this->elements[$outputName] = $element; + } + + // Add pattern or exact element + if(preg_match($hasPatternsRegExp, $elementName)) { + $element->pattern = $this->patternToRegex($elementName); + $this->elementPatterns[] = $element; + } + else { + $this->elements[$elementName] = $element; + } + } + } + } + + /** + * Given an element tag, return the rule structure for that element + * @param string $tag - The element tag + * @return stdClass - The element rule + */ + protected function getRuleForElement($tag) { + if(isset($this->elements[$tag])) { + return $this->elements[$tag]; + } + else foreach($this->elementPatterns as $element) { + if(preg_match($element->pattern, $tag)) return $element; + } + } + + /** + * Given an attribute name, return the rule structure for that attribute + * @param string $name - The attribute name + * @return stdClass - The attribute rule + */ + protected function getRuleForAttribute($elementRule, $name) { + if(isset($elementRule->attributes[$name])) { + return $elementRule->attributes[$name]; + } + else foreach($elementRule->attributePatterns as $attribute) { + if(preg_match($attribute->pattern, $name)) return $attribute; + } + } + + /** + * Given a DOMElement and an element rule, check if that element passes the rule + * @param DOMElement $element - the element to check + * @param stdClass $rule - the rule to check against + * @return bool - true if the element passes (and so can be kept), false if it fails (and so needs stripping) + */ + protected function elementMatchesRule($element, $rule = null) { + // If the rule doesn't exist at all, the element isn't allowed + if(!$rule) return false; + + // If the rule has attributes required, check them to see if this element has at least one + if($rule->attributesRequired) { + $hasMatch = false; + + foreach($rule->attributesRequired as $attr) { + if($element->getAttribute($attr)) { + $hasMatch = true; + break; + } + } + + if(!$hasMatch) return false; + } + + // If the rule says to remove empty elements, and this element is empty, remove it + if($rule->removeEmpty && !$element->firstChild) return false; + + // No further tests required, element passes + return true; + } + + /** + * Given a DOMAttr and an attribute rule, check if that attribute passes the rule + * @param DOMAttr $attr - the attribute to check + * @param stdClass $rule - the rule to check against + * @return bool - true if the attribute passes (and so can be kept), false if it fails (and so needs stripping) + */ + protected function attributeMatchesRule($attr, $rule = null) { + // If the rule doesn't exist at all, the attribute isn't allowed + if(!$rule) return false; + + // If the rule has a set of valid values, check them to see if this attribute is one + if(isset($rule->validValues) && !in_array($attr->value, $rule->validValues)) return false; + + // No further tests required, attribute passes + return true; + } + + /** + * Given an SS_HTMLValue instance, will remove and elements and attributes that are + * not explicitly included in the whitelist passed to __construct on instance creation + * + * @param SS_HTMLValue $html - The HTMLValue to remove any non-whitelisted elements & attributes from + */ + public function sanitise (SS_HTMLValue $html) { + if(!$this->elements && !$this->elementPatterns) return; + + $doc = $html->getDocument(); + + foreach($html->query('//body//*') as $el) { + $elementRule = $this->getRuleForElement($el->tagName); + + // If this element isn't allowed, strip it + if(!$this->elementMatchesRule($el, $elementRule)) { + // If it's a script or style, we don't keep contents + if($el->tagName === 'script' || $el->tagName === 'style') { + $el->parentNode->removeChild($el); + } + // Otherwise we replace this node with all it's children + else { + // First, create a new fragment with all of $el's children moved into it + $frag = $doc->createDocumentFragment(); + while($el->firstChild) $frag->appendChild($el->firstChild); + + // Then replace $el with the frags contents (which used to be it's children) + $el->parentNode->replaceChild($frag, $el); + } + } + // Otherwise tidy the element + else { + // First, if we're supposed to pad & this element is empty, fix that + if($elementRule->paddEmpty && !$el->firstChild) { + $el->nodeValue = ' '; + } + + // Then filter out any non-whitelisted attributes + $children = $el->attributes; + $i = $children->length; + while($i--) { + $attr = $children->item($i); + $attributeRule = $this->getRuleForAttribute($elementRule, $attr->name); + + // If this attribute isn't allowed, strip it + if(!$this->attributeMatchesRule($attr, $attributeRule)) { + $el->removeAttributeNode($attr); + } + } + + // Then enforce any default attributes + foreach($elementRule->attributesDefault as $attr => $default) { + if(!$el->getAttribute($attr)) $el->setAttribute($attr, $default); + } + + // And any forced attributes + foreach($elementRule->attributesForced as $attr => $forced) { + $el->setAttribute($attr, $forced); + } + } + } + } + +} diff --git a/tests/forms/HtmlEditorSanitiserTest.php b/tests/forms/HtmlEditorSanitiserTest.php new file mode 100644 index 000000000..d9927c35b --- /dev/null +++ b/tests/forms/HtmlEditorSanitiserTest.php @@ -0,0 +1,57 @@ +Leave Alone

Strip parentBut keep children in order
', + '

Leave Alone

Strip parentBut keep children in order', + 'Non-whitelisted elements are stripped, but children are kept' + ), + array( + 'p,strong', + '
A B
Nested elements are still filtered
C
D
', + 'A B Nested elements are still filtered C D', + 'Non-whitelisted elements are stripped even when children of non-whitelisted elements' + ), + array( + 'p', + '

Keep

', + '

Keep

', + 'Non-whitelisted script elements are totally stripped, including any children' + ), + array( + 'p[id]', + '

Test

', + '

Test

', + 'Non-whitelisted attributes are stripped' + ), + array( + 'p[default1=default1|default2=default2|force1:force1|force2:force2]', + '

Test

', + '

Test

', + 'Default attributes are set when not present in input, forced attributes are always set' + ) + ); + + $config = HtmlEditorConfig::get('htmleditorsanitisertest'); + + foreach($tests as $test) { + list($validElements, $input, $output, $desc) = $test; + + $config->setOptions(array('valid_elements' => $validElements)); + $sanitiser = new HtmlEditorSanitiser($config); + + $htmlValue = Injector::inst()->create('HTMLValue', $input); + $sanitiser->sanitise($htmlValue); + + $this->assertEquals($output, $htmlValue->getContent(), $desc); + } + } + +} \ No newline at end of file From a862b4da99326e8b937796cbe971cb073b593065 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 4 Jul 2013 09:26:53 +1200 Subject: [PATCH 47/54] BUG Fixed missing allowed_actions on UploadField_SelectHandler --- forms/UploadField.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/forms/UploadField.php b/forms/UploadField.php index 895793142..a3d265682 100644 --- a/forms/UploadField.php +++ b/forms/UploadField.php @@ -1490,6 +1490,10 @@ class UploadField_SelectHandler extends RequestHandler { '' => 'index', ); + private static $allowed_actions = array( + 'Form' + ); + public function __construct($parent, $folderName = null) { $this->parent = $parent; $this->folderName = $folderName; From 409be9a84077395d5ed31963f16cb4ec68ff52f1 Mon Sep 17 00:00:00 2001 From: Dan Brooks Date: Thu, 4 Jul 2013 16:41:53 +0100 Subject: [PATCH 48/54] Fixed broken github issue tracker link --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index d1e7b1fec..74905b374 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ and [installation from source](http://doc.silverstripe.org/framework/en/installa ## Bugtracker ## -Bugs are tracked on [github.com](https://github.com/silverstripe/framework/issues). +Bugs are tracked on [github.com](https://github.com/silverstripe/silverstripe-framework/issues). Please read our [issue reporting guidelines](http://doc.silverstripe.org/framework/en/misc/contributing/issues). ## Development and Contribution ## From cf20923fd6f1282b90a29bce39dbd9fdd70fd1c6 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Thu, 4 Jul 2013 22:28:13 +0200 Subject: [PATCH 49/54] Postgres compat in SQLQueryTest --- tests/model/SQLQueryTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/model/SQLQueryTest.php b/tests/model/SQLQueryTest.php index d3e64b827..7572f8f86 100755 --- a/tests/model/SQLQueryTest.php +++ b/tests/model/SQLQueryTest.php @@ -378,7 +378,7 @@ class SQLQueryTest extends SapphireTest { $query = new SQLQuery(); $query->setSelect(array('"Name"', '"Meta"')); $query->setFrom('"SQLQueryTest_DO"'); - $query->setOrderBy(array('MAX(Date)')); + $query->setOrderBy(array('MAX("Date")')); $query->setGroupBy(array('"Name"', '"Meta"')); $query->setLimit('1', '1'); From 067a94bd932143d5c6002a61bf32e1a7f4208170 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Thu, 4 Jul 2013 22:46:23 +0200 Subject: [PATCH 50/54] Postgres compat in MemberCsvBulkLoaderTest and GroupTest --- tests/security/GroupTest.php | 29 ++++++++++------------ tests/security/MemberCsvBulkLoaderTest.php | 15 ++++++++--- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/tests/security/GroupTest.php b/tests/security/GroupTest.php index e923dad41..3404fde06 100644 --- a/tests/security/GroupTest.php +++ b/tests/security/GroupTest.php @@ -68,12 +68,12 @@ class GroupTest extends FunctionalTest { $form->saveInto($member); $updatedGroups = $member->Groups(); - $this->assertEquals( - array($adminGroup->ID, $parentGroup->ID), - $updatedGroups->column(), + $this->assertEquals(2, count($updatedGroups->column()), "Adding a toplevel group works" ); - + $this->assertContains($adminGroup->ID, $updatedGroups->column('ID')); + $this->assertContains($parentGroup->ID, $updatedGroups->column('ID')); + // Test unsetting relationship $form->loadDataFrom($member); $checkboxSetField = $form->Fields()->fieldByName('Groups'); @@ -84,11 +84,10 @@ class GroupTest extends FunctionalTest { $form->saveInto($member); $member->flushCache(); $updatedGroups = $member->Groups(); - $this->assertEquals( - array($adminGroup->ID), - $updatedGroups->column(), + $this->assertEquals(1, count($updatedGroups->column()), "Removing a previously added toplevel group works" ); + $this->assertContains($adminGroup->ID, $updatedGroups->column('ID')); // Test adding child group @@ -101,23 +100,21 @@ class GroupTest extends FunctionalTest { $orphanGroup->ParentID = 99999; $orphanGroup->write(); - $this->assertEquals( - array($parentGroup->ID), - $parentGroup->collateAncestorIDs(), + $this->assertEquals(1, count($parentGroup->collateAncestorIDs()), 'Root node only contains itself' ); + $this->assertContains($parentGroup->ID, $parentGroup->collateAncestorIDs()); - $this->assertEquals( - array($childGroup->ID, $parentGroup->ID), - $childGroup->collateAncestorIDs(), + $this->assertEquals(2, count($childGroup->collateAncestorIDs()), 'Contains parent nodes, with child node first' ); + $this->assertContains($parentGroup->ID, $childGroup->collateAncestorIDs()); + $this->assertContains($childGroup->ID, $childGroup->collateAncestorIDs()); - $this->assertEquals( - array($orphanGroup->ID), - $orphanGroup->collateAncestorIDs(), + $this->assertEquals(1, count($orphanGroup->collateAncestorIDs()), 'Orphaned nodes dont contain invalid parent IDs' ); + $this->assertContains($orphanGroup->ID, $orphanGroup->collateAncestorIDs()); } public function testDelete() { diff --git a/tests/security/MemberCsvBulkLoaderTest.php b/tests/security/MemberCsvBulkLoaderTest.php index 7e95f93e1..4b5f79993 100644 --- a/tests/security/MemberCsvBulkLoaderTest.php +++ b/tests/security/MemberCsvBulkLoaderTest.php @@ -41,8 +41,11 @@ class MemberCsvBulkLoaderTest extends SapphireTest { $results = $loader->load($this->getCurrentRelativePath() . '/MemberCsvBulkLoaderTest.csv'); $created = $results->Created()->toArray(); - $this->assertEquals($created[0]->Groups()->column('ID'), array($existinggroup->ID)); - $this->assertEquals($created[1]->Groups()->column('ID'), array($existinggroup->ID)); + $this->assertEquals(1, count($created[0]->Groups()->column('ID'))); + $this->assertContains($existinggroup->ID, $created[0]->Groups()->column('ID')); + + $this->assertEquals(1, count($created[1]->Groups()->column('ID'))); + $this->assertContains($existinggroup->ID, $created[1]->Groups()->column('ID')); } public function testAddToCsvColumnGroupsByCode() { @@ -55,8 +58,12 @@ class MemberCsvBulkLoaderTest extends SapphireTest { $this->assertEquals($newgroup->Title, 'newgroup'); $created = $results->Created()->toArray(); - $this->assertEquals($created[0]->Groups()->column('ID'), array($existinggroup->ID)); - $this->assertEquals($created[1]->Groups()->column('ID'), array($existinggroup->ID, $newgroup->ID)); + $this->assertEquals(1, count($created[0]->Groups()->column('ID'))); + $this->assertContains($existinggroup->ID, $created[0]->Groups()->column('ID')); + + $this->assertEquals(2, count($created[1]->Groups()->column('ID'))); + $this->assertContains($existinggroup->ID, $created[1]->Groups()->column('ID')); + $this->assertContains($newgroup->ID, $created[1]->Groups()->column('ID')); } public function testCleartextPasswordsAreHashedWithDefaultAlgo() { From 2845f76ade316c01d92f0909882b0dc3e9b6b8ed Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Thu, 4 Jul 2013 22:19:42 +0200 Subject: [PATCH 51/54] PHP 5.5 CI, don't allow failures for sqlite3 and postgres --- .travis.yml | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/.travis.yml b/.travis.yml index 9f6c99cb4..defe621bc 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,23 +1,24 @@ language: php -php: - - 5.3 - - 5.4 +php: + - 5.3 env: - - DB=MYSQL CORE_RELEASE=3.0 - - DB=PGSQL CORE_RELEASE=3.0 - - DB=SQLITE3 CORE_RELEASE=3.0 + - DB=MYSQL CORE_RELEASE=3.0 matrix: - exclude: - - php: 5.4 + include: + - php: 5.3 env: DB=PGSQL CORE_RELEASE=3.0 + - php: 5.3 + env: DB=SQLITE CORE_RELEASE=3.0 - php: 5.4 - env: DB=SQLITE3 CORE_RELEASE=3.0 + env: DB=MYSQL CORE_RELEASE=3.0 + - php: 5.5 + env: DB=MYSQL CORE_RELEASE=3.0 allow_failures: - - env: DB=PGSQL CORE_RELEASE=3.0 - - env: DB=SQLITE3 CORE_RELEASE=3.0 + - php: 5.5 + env: DB=MYSQL CORE_RELEASE=3.0 before_script: - phpenv rehash From 9deb11f9a0ee01477af38f2826b2a7583545a0b6 Mon Sep 17 00:00:00 2001 From: Simon Welsh Date: Thu, 20 Dec 2012 13:40:42 +1300 Subject: [PATCH 52/54] Use preg_replace_callback over preg_replace with e modifier --- control/HTTP.php | 25 +++++++++++++++---------- core/Convert.php | 9 ++++++--- email/Mailer.php | 4 +++- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/control/HTTP.php b/control/HTTP.php index ae3b9e43d..2baf4131f 100644 --- a/control/HTTP.php +++ b/control/HTTP.php @@ -60,20 +60,25 @@ class HTTP { if(!is_numeric($tag)) $tagPrefix = "$tag "; else $tagPrefix = ""; - $regExps[] = "/(<{$tagPrefix}[^>]*$attrib *= *\")([^\"]*)(\")/ie"; - $regExps[] = "/(<{$tagPrefix}[^>]*$attrib *= *')([^']*)(')/ie"; - $regExps[] = "/(<{$tagPrefix}[^>]*$attrib *= *)([^\"' ]*)( )/ie"; + $regExps[] = "/(<{$tagPrefix}[^>]*$attrib *= *\")([^\"]*)(\")/i"; + $regExps[] = "/(<{$tagPrefix}[^>]*$attrib *= *')([^']*)(')/i"; + $regExps[] = "/(<{$tagPrefix}[^>]*$attrib *= *)([^\"' ]*)( )/i"; } - $regExps[] = '/(background-image:[^;]*url *\()([^)]+)(\))/ie'; - $regExps[] = '/(background:[^;]*url *\()([^)]+)(\))/ie'; - $regExps[] = '/(list-style-image:[^;]*url *\()([^)]+)(\))/ie'; - $regExps[] = '/(list-style:[^;]*url *\()([^)]+)(\))/ie'; + $regExps[] = '/(background-image:[^;]*url *\()([^)]+)(\))/i'; + $regExps[] = '/(background:[^;]*url *\()([^)]+)(\))/i'; + $regExps[] = '/(list-style-image:[^;]*url *\()([^)]+)(\))/i'; + $regExps[] = '/(list-style:[^;]*url *\()([^)]+)(\))/i'; // Make - $code = 'stripslashes("$1") . (' . str_replace('$URL', 'stripslashes("$2")', $code) . ') . stripslashes("$3")'; - + $callback = function($matches) use($code) { + return + stripslashes($matches[1]) . + str_replace('$URL', stripslashes($matches[2]), $code) . + stripslashes($matches[3]); + }; + foreach($regExps as $regExp) { - $content = preg_replace($regExp, $code, $content); + $content = preg_replace_callback($regExp, $callback, $content); } return $content; diff --git a/core/Convert.php b/core/Convert.php index 3df0b37b7..94c6234c0 100644 --- a/core/Convert.php +++ b/core/Convert.php @@ -244,9 +244,12 @@ class Convert { // Expand hyperlinks if(!$preserveLinks && !$config['PreserveLinks']) { - $data = preg_replace('/]*href\s*=\s*"([^"]*)">(.*?)<\/a>/ie', "Convert::html2raw('\\2').'[\\1]'", - $data); - $data = preg_replace('/]*href\s*=\s*([^ ]*)>(.*?)<\/a>/ie', "Convert::html2raw('\\2').'[\\1]'", $data); + $data = preg_replace_callback('/]*href\s*=\s*"([^"]*)">(.*?)<\/a>/i', function($matches) { + return Convert::html2raw($matches[2]) . "[$matches[1]]"; + }, $data); + $data = preg_replace_callback('/]*href\s*=\s*([^ ]*)>(.*?)<\/a>/i', function($matches) { + return Convert::html2raw($matches[2]) . "[$matches[1]]"; + }, $data); } // Replace images with their alt tags diff --git a/email/Mailer.php b/email/Mailer.php index 1202b1530..c45fdffe8 100644 --- a/email/Mailer.php +++ b/email/Mailer.php @@ -424,7 +424,9 @@ function encodeFileForEmail($file, $destFileName = false, $disposition = NULL, $ function QuotedPrintable_encode($quotprint) { $quotprint = (string)str_replace('\r\n',chr(13).chr(10),$quotprint); $quotprint = (string)str_replace('\n', chr(13).chr(10),$quotprint); - $quotprint = (string)preg_replace("~([\x01-\x1F\x3D\x7F-\xFF])~e", "sprintf('=%02X', ord('\\1'))", $quotprint); + $quotprint = (string)preg_replace_callback("~([\x01-\x1F\x3D\x7F-\xFF])~e", function($matches) { + return sprintf('=%02X', ord($matches[1])); + }, $quotprint); //$quotprint = (string)str_replace('\=0D=0A',"=0D=0A",$quotprint); $quotprint = (string)str_replace('=0D=0A',"\n",$quotprint); $quotprint = (string)str_replace('=0A=0D',"\n",$quotprint); From 11f4b2c620c20e661fa6a6ba23bdd59af17edf76 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 24 Jan 2013 11:35:27 +1300 Subject: [PATCH 53/54] API HTTP::urlRewriter with (string)$code deprecated in 3.1. Fixed regressions and CSS urls. urlRewriter will expect a callable as a second parameter, but will work with the current api and simply raise a deprecation error. HTTP::absoluteURLs now correctly rewrites urls into absolute urls. Resolves introduced in c56a80d6ce HTTP::absoluteURLs now handles additional cases where urls were not translated. Test cases for HTTP::absoluteURLs added for both css and attribute links. Cleaned up replacement expression and improved documentation. --- control/HTTP.php | 64 +++++++++++++++++++++++-------- tests/control/HTTPTest.php | 78 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 16 deletions(-) diff --git a/control/HTTP.php b/control/HTTP.php index 2baf4131f..e400c97db 100644 --- a/control/HTTP.php +++ b/control/HTTP.php @@ -41,20 +41,43 @@ class HTTP { */ public static function absoluteURLs($html) { $html = str_replace('$CurrentPageURL', $_SERVER['REQUEST_URI'], $html); - return HTTP::urlRewriter($html, '(substr($URL,0,1) == "/") ? ( Director::protocolAndHost() . $URL ) :' - . ' ( (preg_match("/^[A-Za-z]+:/", $URL)) ? $URL : Director::absoluteBaseURL() . $URL )' ); + return HTTP::urlRewriter($html, function($url) { + return Director::absoluteURL($url, true); + }); } - /* - * Rewrite all the URLs in the given content, evaluating the given string as PHP code + /** + * Rewrite all the URLs in the given content, evaluating the given string as PHP code. * * Put $URL where you want the URL to appear, however, you can't embed $URL in strings * Some example code: - * '"../../" . $URL' - * 'myRewriter($URL)' - * '(substr($URL,0,1)=="/") ? "../" . substr($URL,1) : $URL' + *
    + *
  • '"../../" . $URL'
  • + *
  • 'myRewriter($URL)'
  • + *
  • '(substr($URL,0,1)=="/") ? "../" . substr($URL,1) : $URL'
  • + *
+ * + * As of 3.2 $code should be a callable which takes a single parameter and returns + * the rewritten URL. e.g. + * + * + * function($url) { + * return Director::absoluteURL($url, true); + * } + * + * + * @param string $content The HTML to search for links to rewrite + * @param string|callable $code Either a string that can evaluate to an expression + * to rewrite links (depreciated), or a callable that takes a single + * parameter and returns the rewritten URL + * @return The content with all links rewritten as per the logic specified in $code */ public static function urlRewriter($content, $code) { + if(!is_callable($code)) { + Deprecation::notice(3.1, 'HTTP::urlRewriter expects a callable as the second parameter'); + } + + // Replace attributes $attribs = array("src","background","a" => "href","link" => "href", "base" => "href"); foreach($attribs as $tag => $attrib) { if(!is_numeric($tag)) $tagPrefix = "$tag "; @@ -64,19 +87,28 @@ class HTTP { $regExps[] = "/(<{$tagPrefix}[^>]*$attrib *= *')([^']*)(')/i"; $regExps[] = "/(<{$tagPrefix}[^>]*$attrib *= *)([^\"' ]*)( )/i"; } - $regExps[] = '/(background-image:[^;]*url *\()([^)]+)(\))/i'; - $regExps[] = '/(background:[^;]*url *\()([^)]+)(\))/i'; - $regExps[] = '/(list-style-image:[^;]*url *\()([^)]+)(\))/i'; - $regExps[] = '/(list-style:[^;]*url *\()([^)]+)(\))/i'; + // Replace css styles + // @todo - http://www.css3.info/preview/multiple-backgrounds/ + $styles = array('background-image', 'background', 'list-style-image', 'list-style', 'content'); + foreach($styles as $style) { + $regExps[] = "/($style:[^;]*url *\(\")([^\"]+)(\"\))/i"; + $regExps[] = "/($style:[^;]*url *\(')([^']+)('\))/i"; + $regExps[] = "/($style:[^;]*url *\()([^\"\)')]+)(\))/i"; + } - // Make + // Callback for regexp replacement $callback = function($matches) use($code) { - return - stripslashes($matches[1]) . - str_replace('$URL', stripslashes($matches[2]), $code) . - stripslashes($matches[3]); + if(is_callable($code)) { + $rewritten = $code($matches[2]); + } else { + // Expose the $URL variable to be used by the $code expression + $URL = $matches[2]; + $rewritten = eval("return ($code);"); + } + return $matches[1] . $rewritten . $matches[3]; }; + // Execute each expression foreach($regExps as $regExp) { $content = preg_replace_callback($regExp, $callback, $content); } diff --git a/tests/control/HTTPTest.php b/tests/control/HTTPTest.php index 5379c7ae9..7cc962c70 100644 --- a/tests/control/HTTPTest.php +++ b/tests/control/HTTPTest.php @@ -120,4 +120,82 @@ class HTTPTest extends SapphireTest { HTTP::get_mime_type(FRAMEWORK_DIR.'/tests/control/files/file.psd')); $this->assertEquals('audio/x-wav', HTTP::get_mime_type(FRAMEWORK_DIR.'/tests/control/files/file.wav')); } + + /** + * Test that absoluteURLs correctly transforms urls within CSS to absolute + */ + public function testAbsoluteURLsCSS() { + $this->withBaseURL('http://www.silverstripe.org/', function($test){ + + // background-image + // Note that using /./ in urls is absolutely acceptable + $test->assertEquals( + '
Content
', + HTTP::absoluteURLs('
Content
') + ); + + // background + $test->assertEquals( + '
Content
', + HTTP::absoluteURLs('
Content
') + ); + + // list-style-image + $test->assertEquals( + '
Content
', + HTTP::absoluteURLs('
Content
') + ); + + // list-style + $test->assertEquals( + '
Content
', + HTTP::absoluteURLs('
Content
') + ); + }); + } + + /** + * Test that absoluteURLs correctly transforms urls within html attributes to absolute + */ + public function testAbsoluteURLsAttributes() { + $this->withBaseURL('http://www.silverstripe.org/', function($test){ + + // links + $test->assertEquals( + 'SS Blog', + HTTP::absoluteURLs('SS Blog') + ); + + // background + // Note that using /./ in urls is absolutely acceptable + $test->assertEquals( + '
SS Blog
', + HTTP::absoluteURLs('
SS Blog
') + ); + + // image + $test->assertEquals( + '', + HTTP::absoluteURLs('') + ); + + // link + $test->assertEquals( + '', + HTTP::absoluteURLs('') + ); + }); + } + + /** + * Run a test while mocking the base url with the provided value + * @param string $url The base URL to use for this test + * @param callable $callback The test to run + */ + protected function withBaseURL($url, $callback) { + $oldBase = Director::$alternateBaseURL; + Director::setBaseURL($url); + $callback($this); + Director::setBaseURL($oldBase); + } } From fb457e47eb2c2c17615cf12d39e01a6a94248f56 Mon Sep 17 00:00:00 2001 From: Simon Welsh Date: Fri, 5 Jul 2013 09:45:30 +1200 Subject: [PATCH 54/54] Removes PHP 5.5 from allowed failures --- .travis.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index defe621bc..6093b7bed 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,9 +16,6 @@ matrix: env: DB=MYSQL CORE_RELEASE=3.0 - php: 5.5 env: DB=MYSQL CORE_RELEASE=3.0 - allow_failures: - - php: 5.5 - env: DB=MYSQL CORE_RELEASE=3.0 before_script: - phpenv rehash