From 4e9c74243dd9ac11510e77628f59c0135a2a7ddb Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Tue, 7 Feb 2023 11:56:04 +1300 Subject: [PATCH 01/20] API Deprecate code --- src/Control/Director.php | 2 + src/Control/Email/Email.php | 18 +++++-- src/Control/HTTP.php | 2 + src/Core/BaseKernel.php | 6 ++- src/Core/CustomMethods.php | 10 +++- src/Core/Extensible.php | 5 +- src/Dev/Tasks/MigrateFileTask.php | 8 ++- src/Dev/TestMailer.php | 8 ++- src/ORM/Connect/MySQLQuery.php | 30 ++++++++--- src/ORM/Connect/MySQLStatement.php | 25 ++++++--- src/ORM/Connect/PDOConnector.php | 13 +++++ src/ORM/Connect/PDOQuery.php | 12 ++++- src/ORM/Connect/PDOStatementHandle.php | 10 ++++ src/ORM/Connect/Query.php | 72 ++++++++++++++++++-------- src/ORM/DataList.php | 5 ++ src/ORM/Map_Iterator.php | 10 ++++ 16 files changed, 190 insertions(+), 46 deletions(-) diff --git a/src/Control/Director.php b/src/Control/Director.php index 880561253..ca26754d9 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -73,6 +73,7 @@ class Director implements TemplateGlobalProvider /** * @config * @var string + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ private static $alternate_base_folder; @@ -82,6 +83,7 @@ class Director implements TemplateGlobalProvider * * @config * @var bool|null + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ private static $alternate_public_dir = null; diff --git a/src/Control/Email/Email.php b/src/Control/Email/Email.php index 766bf8f7c..6866331b3 100644 --- a/src/Control/Email/Email.php +++ b/src/Control/Email/Email.php @@ -115,7 +115,9 @@ class Email extends ViewableData */ public static function getSendAllEmailsTo() { - return static::mergeConfiguredEmails('send_all_emails_to', 'SS_SEND_ALL_EMAILS_TO'); + return Deprecation::withNoReplacement(function () { + return static::mergeConfiguredEmails('send_all_emails_to', 'SS_SEND_ALL_EMAILS_TO'); + }); } /** @@ -125,7 +127,9 @@ class Email extends ViewableData */ public static function getCCAllEmailsTo() { - return static::mergeConfiguredEmails('cc_all_emails_to', 'SS_CC_ALL_EMAILS_TO'); + return Deprecation::withNoReplacement(function () { + return static::mergeConfiguredEmails('cc_all_emails_to', 'SS_CC_ALL_EMAILS_TO'); + }); } /** @@ -135,7 +139,9 @@ class Email extends ViewableData */ public static function getBCCAllEmailsTo() { - return static::mergeConfiguredEmails('bcc_all_emails_to', 'SS_BCC_ALL_EMAILS_TO'); + return Deprecation::withNoReplacement(function () { + return static::mergeConfiguredEmails('bcc_all_emails_to', 'SS_BCC_ALL_EMAILS_TO'); + }); } /** @@ -145,7 +151,9 @@ class Email extends ViewableData */ public static function getSendAllEmailsFrom() { - return static::mergeConfiguredEmails('send_all_emails_from', 'SS_SEND_ALL_EMAILS_FROM'); + return Deprecation::withNoReplacement(function () { + return static::mergeConfiguredEmails('send_all_emails_from', 'SS_SEND_ALL_EMAILS_FROM'); + }); } /** @@ -154,9 +162,11 @@ class Email extends ViewableData * @param string $config Config key * @param string $env Env variable key * @return array Array of email addresses + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ protected static function mergeConfiguredEmails($config, $env) { + Deprecation::notice('4.13.0', 'Will be removed without equivalent functionality to replace it'); // Normalise config list $normalised = []; $source = (array)static::config()->get($config); diff --git a/src/Control/HTTP.php b/src/Control/HTTP.php index cce77aec4..ca07fc57f 100644 --- a/src/Control/HTTP.php +++ b/src/Control/HTTP.php @@ -576,9 +576,11 @@ class HTTP * Return static variable cache_age in second * * @return int + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ public static function get_cache_age() { + Deprecation::notice('4.13.0', 'Will be removed without equivalent functionality to replace it'); return self::$cache_age; } } diff --git a/src/Core/BaseKernel.php b/src/Core/BaseKernel.php index 3706f94d6..fd256b0a0 100644 --- a/src/Core/BaseKernel.php +++ b/src/Core/BaseKernel.php @@ -276,7 +276,9 @@ abstract class BaseKernel implements Kernel } // Check saved session - $env = $this->sessionEnvironment(); + $env = Deprecation::withNoReplacement(function () { + return $this->sessionEnvironment(); + }); if ($env) { return $env; } @@ -293,9 +295,11 @@ abstract class BaseKernel implements Kernel * Check or update any temporary environment specified in the session. * * @return null|string + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ protected function sessionEnvironment() { + Deprecation::notice('4.13.0', 'Will be removed without equivalent functionality to replace it.'); if (!$this->booted) { // session is not initialyzed yet, neither is manifest return null; diff --git a/src/Core/CustomMethods.php b/src/Core/CustomMethods.php index ee6ea63ef..9f9c4c9ee 100644 --- a/src/Core/CustomMethods.php +++ b/src/Core/CustomMethods.php @@ -193,9 +193,11 @@ trait CustomMethods /** * @param object $extension * @return array + * @deprecated 4.13.0 Will be replaced by findMethodsFrom() in CMS 5 */ protected function findMethodsFromExtension($extension) { + Deprecation::notice('4.13.0', 'Will be replaced by findMethodsFrom() in CMS 5'); if (method_exists($extension, 'allMethodNames')) { if ($extension instanceof Extension) { try { @@ -236,7 +238,9 @@ trait CustomMethods ); } - $methods = $this->findMethodsFromExtension($extension); + $methods = Deprecation::withNoReplacement(function () use ($extension) { + return $this->findMethodsFromExtension($extension); + }); if ($methods) { if ($extension instanceof Extension) { Deprecation::notice( @@ -279,7 +283,9 @@ trait CustomMethods ); } - $methods = $this->findMethodsFromExtension($extension); + $methods = Deprecation::withNoReplacement(function () use ($extension) { + return $this->findMethodsFromExtension($extension); + }); if ($methods) { foreach ($methods as $method) { if (!isset(self::$extra_methods[$class][$method])) { diff --git a/src/Core/Extensible.php b/src/Core/Extensible.php index f041caf4a..3cd80b9e8 100644 --- a/src/Core/Extensible.php +++ b/src/Core/Extensible.php @@ -131,7 +131,10 @@ trait Extensible { $extensions = $this->getExtensionInstances(); foreach ($extensions as $extensionClass => $extensionInstance) { - foreach ($this->findMethodsFromExtension($extensionInstance) as $method) { + $methods = Deprecation::withNoReplacement(function () use ($extensionInstance) { + return $this->findMethodsFromExtension($extensionInstance); + }); + foreach ($methods as $method) { $this->addCallbackMethod($method, function ($inst, $args) use ($method, $extensionClass) { /** @var Extensible $inst */ $extension = $inst->getExtensionInstance($extensionClass); diff --git a/src/Dev/Tasks/MigrateFileTask.php b/src/Dev/Tasks/MigrateFileTask.php index 797e58ec3..be579245d 100644 --- a/src/Dev/Tasks/MigrateFileTask.php +++ b/src/Dev/Tasks/MigrateFileTask.php @@ -57,7 +57,13 @@ class MigrateFileTask extends BuildTask public function __construct() { - Deprecation::notice('4.12.0', 'Will be removed without equivalent functionality to replace it', Deprecation::SCOPE_CLASS); + Deprecation::withNoReplacement(function () { + Deprecation::notice( + '4.13.0', + 'Will be removed without equivalent functionality to replace it', + Deprecation::SCOPE_CLASS + ); + }); parent::__construct(); } diff --git a/src/Dev/TestMailer.php b/src/Dev/TestMailer.php index 44627e62c..e9363121a 100644 --- a/src/Dev/TestMailer.php +++ b/src/Dev/TestMailer.php @@ -4,11 +4,13 @@ namespace SilverStripe\Dev; use SilverStripe\Control\Email\Mailer; use Swift_Attachment; +use SilverStripe\Dev\Deprecation; class TestMailer implements Mailer { /** * @var array + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ protected $emailsSent = []; @@ -58,7 +60,9 @@ class TestMailer implements Mailer $serialised['HtmlContent'] = $htmlContent; } - $this->saveEmail($serialised); + Deprecation::withNoReplacement(function () use ($serialised) { + $this->saveEmail($serialised); + }); return true; } @@ -67,9 +71,11 @@ class TestMailer implements Mailer * Save a single email to the log * * @param array $data A map of information about the email + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ protected function saveEmail($data) { + Deprecation::notice('4.13.0', 'Will be removed without equivalent functionality to replace it'); $this->emailsSent[] = $data; } diff --git a/src/ORM/Connect/MySQLQuery.php b/src/ORM/Connect/MySQLQuery.php index c81138643..fece476aa 100644 --- a/src/ORM/Connect/MySQLQuery.php +++ b/src/ORM/Connect/MySQLQuery.php @@ -2,6 +2,8 @@ namespace SilverStripe\ORM\Connect; +use SilverStripe\Dev\Deprecation; + /** * A result-set from a MySQL database (using MySQLiConnector) * Note that this class is only used for the results of non-prepared statements @@ -45,16 +47,22 @@ class MySQLQuery extends Query } } + /** + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 + */ public function seek($row) { - if (is_object($this->handle)) { - // Fix for https://github.com/silverstripe/silverstripe-framework/issues/9097 without breaking the seek() API - $this->handle->data_seek($row); - $result = $this->nextRecord(); - $this->handle->data_seek($row); - return $result; - } - return null; + return Deprecation::withNoReplacement(function () use ($row) { + Deprecation::notice('4.13.0', 'Will be replaced by getIterator() in CMS 5'); + if (is_object($this->handle)) { + // Fix for https://github.com/silverstripe/silverstripe-framework/issues/9097 without breaking the seek() API + $this->handle->data_seek($row); + $result = $this->nextRecord(); + $this->handle->data_seek($row); + return $result; + } + return null; + }); } public function numRecords() @@ -65,8 +73,14 @@ class MySQLQuery extends Query return null; } + /** + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 + */ public function nextRecord() { + Deprecation::withNoReplacement(function () { + Deprecation::notice('4.13.0', 'Will be replaced by getIterator() in CMS 5'); + }); $floatTypes = [MYSQLI_TYPE_FLOAT, MYSQLI_TYPE_DOUBLE, MYSQLI_TYPE_DECIMAL, MYSQLI_TYPE_NEWDECIMAL]; if (is_object($this->handle) && ($row = $this->handle->fetch_array(MYSQLI_NUM))) { diff --git a/src/ORM/Connect/MySQLStatement.php b/src/ORM/Connect/MySQLStatement.php index 7bc54765f..06c76b7d1 100644 --- a/src/ORM/Connect/MySQLStatement.php +++ b/src/ORM/Connect/MySQLStatement.php @@ -4,6 +4,7 @@ namespace SilverStripe\ORM\Connect; use mysqli_result; use mysqli_stmt; +use SilverStripe\Dev\Deprecation; /** * Provides a record-view for mysqli prepared statements @@ -102,15 +103,21 @@ class MySQLStatement extends Query $this->currentRecord = false; } + /** + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 + */ public function seek($row) { - $this->rowNum = $row - 1; + return Deprecation::withNoReplacement(function () use ($row) { + Deprecation::notice('4.13.0', 'Will be replaced by getIterator() in CMS 5'); + $this->rowNum = $row - 1; - // Fix for https://github.com/silverstripe/silverstripe-framework/issues/9097 without breaking the seek() API - $this->statement->data_seek($row); - $result = $this->next(); - $this->statement->data_seek($row); - return $result; + // Fix for https://github.com/silverstripe/silverstripe-framework/issues/9097 without breaking the seek() API + $this->statement->data_seek($row); + $result = $this->next(); + $this->statement->data_seek($row); + return $result; + }); } public function numRecords() @@ -118,8 +125,14 @@ class MySQLStatement extends Query return $this->statement->num_rows(); } + /** + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 + */ public function nextRecord() { + Deprecation::withNoReplacement(function () { + Deprecation::notice('4.13.0', 'Will be replaced by getIterator() in CMS 5'); + }); // Skip data if out of data if (!$this->statement->fetch()) { return false; diff --git a/src/ORM/Connect/PDOConnector.php b/src/ORM/Connect/PDOConnector.php index c275d5d4f..cd40ea6c3 100644 --- a/src/ORM/Connect/PDOConnector.php +++ b/src/ORM/Connect/PDOConnector.php @@ -11,6 +11,8 @@ use PDOException; /** * PDO driver database connector + * + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ class PDOConnector extends DBConnector implements TransactionManager { @@ -87,6 +89,17 @@ class PDOConnector extends DBConnector implements TransactionManager */ protected $inTransaction = false; + public function __construct() + { + Deprecation::withNoReplacement(function () { + Deprecation::notice( + '4.13.0', + 'Will be removed without equivalent functionality to replace it', + Deprecation::SCOPE_CLASS + ); + }); + } + /** * Flush all prepared statements */ diff --git a/src/ORM/Connect/PDOQuery.php b/src/ORM/Connect/PDOQuery.php index 7180de28d..fe3e144ab 100644 --- a/src/ORM/Connect/PDOQuery.php +++ b/src/ORM/Connect/PDOQuery.php @@ -2,8 +2,12 @@ namespace SilverStripe\ORM\Connect; +use SilverStripe\Dev\Deprecation; + /** * A result-set from a PDO database. + * + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ class PDOQuery extends Query { @@ -18,10 +22,16 @@ class PDOQuery extends Query */ public function __construct(PDOStatementHandle $statement) { + Deprecation::withNoReplacement(function () { + Deprecation::notice( + '4.13.0', + 'Will be removed without equivalent functionality to replace it', + Deprecation::SCOPE_CLASS + ); + }); // Since no more than one PDOStatement for any one connection can be safely // traversed, each statement simply requests all rows at once for safety. // This could be re-engineered to call fetchAll on an as-needed basis - $this->results = $statement->typeCorrectedFetchAll(); $statement->closeCursor(); } diff --git a/src/ORM/Connect/PDOStatementHandle.php b/src/ORM/Connect/PDOStatementHandle.php index b2ad1971f..850d32cca 100644 --- a/src/ORM/Connect/PDOStatementHandle.php +++ b/src/ORM/Connect/PDOStatementHandle.php @@ -4,6 +4,7 @@ namespace SilverStripe\ORM\Connect; use PDO; use PDOStatement; +use SilverStripe\Dev\Deprecation; /** * A handle to a PDOStatement, with cached column metadata, and type conversion @@ -11,6 +12,8 @@ use PDOStatement; * Column metadata can't be fetched from a native PDOStatement after multiple calls in some DB backends, * so we wrap in this handle object, which also takes care of tidying up content types to keep in line * with the SilverStripe 4.4+ type expectations. + * + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ class PDOStatementHandle { @@ -36,6 +39,13 @@ class PDOStatementHandle */ public function __construct(PDOStatement $statement) { + Deprecation::withNoReplacement(function () { + Deprecation::notice( + '4.13.0', + 'Will be removed without equivalent functionality to replace it', + Deprecation::SCOPE_CLASS + ); + }); $this->statement = $statement; } diff --git a/src/ORM/Connect/Query.php b/src/ORM/Connect/Query.php index af9e0c3a5..5aeb673f7 100644 --- a/src/ORM/Connect/Query.php +++ b/src/ORM/Connect/Query.php @@ -4,6 +4,7 @@ namespace SilverStripe\ORM\Connect; use SilverStripe\Core\Convert; use Iterator; +use SilverStripe\Dev\Deprecation; /** * Abstract query-result class. A query result provides an iterator that returns a map for each record of a query @@ -34,6 +35,7 @@ abstract class Query implements Iterator * The current record in the iterator. * * @var array + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 */ protected $currentRecord = null; @@ -41,6 +43,7 @@ abstract class Query implements Iterator * The number of the current row in the iterator. * * @var int + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 */ protected $rowNum = -1; @@ -48,6 +51,7 @@ abstract class Query implements Iterator * Flag to keep track of whether iteration has begun, to prevent unnecessary seeks * * @var bool + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 */ protected $queryHasBegun = false; @@ -169,52 +173,68 @@ abstract class Query implements Iterator * Makes use of {@link seek()} and {@link numRecords()}, takes care of the plumbing. * * @return void + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 */ #[\ReturnTypeWillChange] public function rewind() { - if ($this->queryHasBegun && $this->numRecords() > 0) { - $this->queryHasBegun = false; - $this->currentRecord = null; - $this->seek(0); - } + Deprecation::withNoReplacement(function () { + Deprecation::notice('4.13.0', 'Will be replaced by getIterator() in CMS 5'); + if ($this->queryHasBegun && $this->numRecords() > 0) { + $this->queryHasBegun = false; + $this->currentRecord = null; + $this->seek(0); + } + }); } /** * Iterator function implementation. Return the current item of the iterator. * * @return array + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 */ #[\ReturnTypeWillChange] public function current() { - if (!$this->currentRecord) { - return $this->next(); - } else { - return $this->currentRecord; - } + return Deprecation::withNoReplacement(function () { + Deprecation::notice('4.13.0', 'Will be replaced by getIterator() in CMS 5'); + if (!$this->currentRecord) { + return $this->next(); + } else { + return $this->currentRecord; + } + }); } /** * Iterator function implementation. Return the first item of this iterator. * * @return array + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 */ public function first() { - $this->rewind(); - return $this->current(); + return Deprecation::withNoReplacement(function () { + Deprecation::notice('4.13.0', 'Will be replaced by getIterator() in CMS 5'); + $this->rewind(); + return $this->current(); + }); } /** * Iterator function implementation. Return the row number of the current item. * * @return int + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 */ #[\ReturnTypeWillChange] public function key() { - return $this->rowNum; + return Deprecation::withNoReplacement(function () { + Deprecation::notice('4.13.0', 'Will be replaced by getIterator() in CMS 5'); + return $this->rowNum; + }); } /** @@ -222,34 +242,43 @@ abstract class Query implements Iterator * Makes use of {@link nextRecord()}, takes care of the plumbing. * * @return array + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 */ #[\ReturnTypeWillChange] public function next() { - $this->queryHasBegun = true; - $this->currentRecord = $this->nextRecord(); - $this->rowNum++; - return $this->currentRecord; + return Deprecation::withNoReplacement(function () { + Deprecation::notice('4.13.0', 'Will be replaced by getIterator() in CMS 5'); + $this->queryHasBegun = true; + $this->currentRecord = $this->nextRecord(); + $this->rowNum++; + return $this->currentRecord; + }); } /** * Iterator function implementation. Check if the iterator is pointing to a valid item. * * @return bool + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ #[\ReturnTypeWillChange] public function valid() { - if (!$this->queryHasBegun) { - $this->next(); - } - return $this->currentRecord !== false; + return Deprecation::withNoReplacement(function () { + Deprecation::notice('4.13.0', 'Will be removed without equivalent functionality to replace it.'); + if (!$this->queryHasBegun) { + $this->next(); + } + return $this->currentRecord !== false; + }); } /** * Return the next record in the query result. * * @return array + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 */ abstract public function nextRecord(); @@ -265,6 +294,7 @@ abstract class Query implements Iterator * * @param int $rowNum Row number to go to. * @return array + * @deprecated 4.13.0 Will be replaced by getIterator() in CMS 5 */ abstract public function seek($rowNum); } diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index 72dfaf02e..93c48fb7b 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -11,6 +11,7 @@ use ArrayIterator; use Exception; use InvalidArgumentException; use LogicException; +use SilverStripe\Dev\Deprecation; /** * Implements a "lazy loading" DataObjectSet. @@ -785,9 +786,13 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li * Returns a generator for this DataList * * @return \Generator&DataObject[] + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ public function getGenerator() { + Deprecation::withNoReplacement(function () { + Deprecation::notice('4.13.0', 'Will be removed without equivalent functionality to replace it'); + }); $query = $this->dataQuery->query()->execute(); while ($row = $query->record()) { diff --git a/src/ORM/Map_Iterator.php b/src/ORM/Map_Iterator.php index d645ca07d..68627639c 100644 --- a/src/ORM/Map_Iterator.php +++ b/src/ORM/Map_Iterator.php @@ -3,9 +3,12 @@ namespace SilverStripe\ORM; use Iterator; +use SilverStripe\Dev\Deprecation; /** * Builds a map iterator around an Iterator. Called by Map + * + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ class Map_Iterator implements Iterator { @@ -35,6 +38,13 @@ class Map_Iterator implements Iterator */ public function __construct(Iterator $items, $keyField, $titleField, $firstItems = null, $lastItems = null) { + Deprecation::withNoReplacement(function () { + Deprecation::notice( + '4.13.0', + 'Will be removed without equivalent functionality to replace it', + Deprecation::SCOPE_CLASS + ); + }); $this->items = $items; $this->keyField = $keyField; $this->titleField = $titleField; From 3a14aafc7f10de60adabf72b9788515255168aa3 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 2 Feb 2023 14:04:34 +1300 Subject: [PATCH 02/20] API Deprecate Diff in favour of CMS5's HtmlDiff --- src/View/Parsers/Diff.php | 23 +++++++++++++++++++++++ tests/php/View/Parsers/DiffTest.php | 25 ++++++++++++++++++++++--- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/View/Parsers/Diff.php b/src/View/Parsers/Diff.php index 3521fa43c..1e8060656 100644 --- a/src/View/Parsers/Diff.php +++ b/src/View/Parsers/Diff.php @@ -5,16 +5,28 @@ namespace SilverStripe\View\Parsers; use InvalidArgumentException; use SilverStripe\Core\Convert; use SilverStripe\Core\Injector\Injector; +use SilverStripe\Dev\Deprecation; require_once 'difflib/difflib.php'; /** * Class representing a 'diff' between two sequences of strings. + * + * @deprecated 4.13.0 Will be replaced with SilverStripe\View\Parsers\HtmlDiff */ class Diff extends \Diff { public static $html_cleaner_class = null; + /** + * @inheritDoc + */ + public function __construct($from_lines, $to_lines) + { + Deprecation::notice('4.13.0', 'Will be replaced with SilverStripe\View\Parsers\HtmlDiff', Deprecation::SCOPE_CLASS); + parent::__construct($from_lines, $to_lines); + } + /** * Attempt to clean invalid HTML, which messes up diffs. * This cleans code if possible, using an instance of HTMLCleaner @@ -26,9 +38,12 @@ class Diff extends \Diff * @param HTMLCleaner $cleaner Optional instance of a HTMLCleaner class to * use, overriding self::$html_cleaner_class * @return mixed|string + * + * @deprecated 4.13.0 Will be removed without equivalent functionality */ public static function cleanHTML($content, $cleaner = null) { + Deprecation::notice('4.13.0', 'Will be removed without equivalent functionality'); if (!$cleaner) { if (self::$html_cleaner_class && class_exists(self::$html_cleaner_class)) { $cleaner = Injector::inst()->create(self::$html_cleaner_class); @@ -57,9 +72,13 @@ class Diff extends \Diff * @param string $to * @param bool $escape * @return string + * + * @deprecated 4.13.0 Will be replaced with SilverStripe\View\Parsers\HtmlDiff::compareHTML() */ public static function compareHTML($from, $to, $escape = false) { + Deprecation::notice('4.13.0', 'Will be replaced with SilverStripe\View\Parsers\HtmlDiff::compareHTML()'); + // First split up the content into words and tags $set1 = self::getHTMLChunks($from); $set2 = self::getHTMLChunks($to); @@ -163,9 +182,13 @@ class Diff extends \Diff /** * @param string|bool|array $content If passed as an array, values will be concatenated with a comma. * @return array + * + * @deprecated 4.13.0 Will be removed without equivalent functionality */ public static function getHTMLChunks($content) { + Deprecation::notice('4.13.0', 'Will be removed without equivalent functionality'); + if ($content && !is_string($content) && !is_array($content) && !is_numeric($content) && !is_bool($content)) { throw new InvalidArgumentException('$content parameter needs to be a string or array'); } diff --git a/tests/php/View/Parsers/DiffTest.php b/tests/php/View/Parsers/DiffTest.php index 3eab9c66c..ce8998bc9 100644 --- a/tests/php/View/Parsers/DiffTest.php +++ b/tests/php/View/Parsers/DiffTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\View\Tests\Parsers; +use SilverStripe\Dev\Deprecation; use SilverStripe\Dev\SapphireTest; use SilverStripe\View\Parsers\Diff; @@ -13,6 +14,10 @@ class DiffTest extends SapphireTest */ public function testTableDiff() { + if (Deprecation::isEnabled()) { + $this->markTestSkipped('Test calls deprecated code'); + } + if (!class_exists('DOMDocument')) { $this->markTestSkipped('"DOMDocument" required'); return; @@ -47,7 +52,9 @@ class DiffTest extends SapphireTest "; $expected = "" . $to . "" . "" . $from . ""; - $compare = Diff::compareHTML($from, $to); + $compare = Deprecation::withNoReplacement(function () use ($from, $to) { + return Diff::compareHTML($from, $to); + }); // Very hard to debug this way, wouldn't need to do this if PHP had an *actual* DOM parsing lib, // and not just the poor excuse that is DOMDocument @@ -62,6 +69,10 @@ class DiffTest extends SapphireTest */ public function testLegacyEachStatement() { + if (Deprecation::isEnabled()) { + $this->markTestSkipped('Test calls deprecated code'); + } + $sentenceOne = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit.'; $sentenceTwo = @@ -73,17 +84,25 @@ class DiffTest extends SapphireTest // We're cheating our test a little bit here, because depending on what HTML cleaner you have, you'll get // spaces added or not added around the tags. $expected = "/^ *$sentenceOne<\/del> *$sentenceTwo *$sentenceOne<\/ins> *$/"; - $actual = Diff::compareHTML($from, $to); + $actual = Deprecation::withNoReplacement(function () use ($from, $to) { + return Diff::compareHTML($from, $to); + }); $this->assertMatchesRegularExpression($expected, $actual); } public function testDiffArray() { + if (Deprecation::isEnabled()) { + $this->markTestSkipped('Test calls deprecated code'); + } + $from = ['Lorem', ['array here please ignore'], 'ipsum dolor']; $to = 'Lorem,ipsum'; $expected = "/^Lorem,ipsum *dolor<\/del> *$/"; - $actual = Diff::compareHTML($from, $to); + $actual = Deprecation::withNoReplacement(function () use ($from, $to) { + return Diff::compareHTML($from, $to); + }); $this->assertMatchesRegularExpression($expected, $actual); } From 54fc4ee9d2bf812d166e225cab3777916c72f6dc Mon Sep 17 00:00:00 2001 From: Florian Thoma Date: Thu, 9 Feb 2023 17:09:48 +1100 Subject: [PATCH 03/20] fix directory separator in i18nTextCollector on Windows (#10681) * fix directory separator in i18nTextCollector for Windows * fix typo --- src/i18n/TextCollection/i18nTextCollector.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/i18n/TextCollection/i18nTextCollector.php b/src/i18n/TextCollection/i18nTextCollector.php index e7d5e95fb..284c3baa0 100644 --- a/src/i18n/TextCollection/i18nTextCollector.php +++ b/src/i18n/TextCollection/i18nTextCollector.php @@ -457,7 +457,7 @@ class i18nTextCollector $this->getWriter()->write( $entities, $this->defaultLocale, - $this->baseSavePath . '/' . $module->getRelativePath() + $this->baseSavePath . DIRECTORY_SEPARATOR . $module->getRelativePath() ); return $this; } @@ -511,25 +511,25 @@ class i18nTextCollector $modulePath = $module->getPath(); // Search all .ss files in themes - if (stripos($module->getRelativePath() ?? '', 'themes/') === 0) { + if (stripos($module->getRelativePath() ?? '', 'themes' . DIRECTORY_SEPARATOR) === 0) { return $this->getFilesRecursive($modulePath, null, 'ss'); } // If non-standard module structure, search all root files - if (!is_dir("{$modulePath}/code") && !is_dir("{$modulePath}/src")) { + if (!is_dir($modulePath . DIRECTORY_SEPARATOR . 'code') && !is_dir($modulePath . DIRECTORY_SEPARATOR . 'src')) { return $this->getFilesRecursive($modulePath); } // Get code files - if (is_dir("{$modulePath}/src")) { - $files = $this->getFilesRecursive("{$modulePath}/src", null, 'php'); + if (is_dir($modulePath . DIRECTORY_SEPARATOR . 'src')) { + $files = $this->getFilesRecursive($modulePath . DIRECTORY_SEPARATOR . 'src', null, 'php'); } else { - $files = $this->getFilesRecursive("{$modulePath}/code", null, 'php'); + $files = $this->getFilesRecursive($modulePath . DIRECTORY_SEPARATOR . 'code', null, 'php'); } // Search for templates in this module - if (is_dir("{$modulePath}/templates")) { - $templateFiles = $this->getFilesRecursive("{$modulePath}/templates", null, 'ss'); + if (is_dir($modulePath . DIRECTORY_SEPARATOR . 'templates')) { + $templateFiles = $this->getFilesRecursive($modulePath . DIRECTORY_SEPARATOR . 'templates', null, 'ss'); } else { $templateFiles = $this->getFilesRecursive($modulePath, null, 'ss'); } @@ -904,7 +904,7 @@ class i18nTextCollector $fileList = []; } // Skip ignored folders - if (is_file("{$folder}/_manifest_exclude") || preg_match($folderExclude ?? '', $folder ?? '')) { + if (is_file($folder . DIRECTORY_SEPARATOR . '_manifest_exclude') || preg_match($folderExclude ?? '', $folder ?? '')) { return $fileList; } From ab566b0a156376020dc25e6754c9d07ca12c2d56 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Wed, 15 Feb 2023 13:26:36 +1300 Subject: [PATCH 04/20] API Add new deprecation notices. (#10691) These are removed in CMS 5. --- src/Core/Manifest/Module.php | 3 +++ src/Dev/CSVParser.php | 3 ++- src/Dev/CsvBulkLoader.php | 10 +++++----- src/Dev/TestSession_STResponseWrapper.php | 8 ++++++++ tests/php/Dev/CSVParserTest.php | 4 ++++ 5 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/Core/Manifest/Module.php b/src/Core/Manifest/Module.php index 77dd61ed2..7f4ff1580 100644 --- a/src/Core/Manifest/Module.php +++ b/src/Core/Manifest/Module.php @@ -23,16 +23,19 @@ class Module implements Serializable /** * Return value of getCIConfig() when module uses PHPUNit 9 + * @deprecated 4.13.0 Will be removed without equivalent functionality */ const CI_PHPUNIT_NINE = 'CI_PHPUNIT_NINE'; /** * Return value of getCIConfig() when module uses PHPUNit 5 + * @deprecated 4.13.0 Will be removed without equivalent functionality */ const CI_PHPUNIT_FIVE = 'CI_PHPUNIT_FIVE'; /** * Return value of getCIConfig() when module does not use any CI + * @deprecated 4.13.0 Will be removed without equivalent functionality */ const CI_UNKNOWN = 'CI_UNKNOWN'; diff --git a/src/Dev/CSVParser.php b/src/Dev/CSVParser.php index fda09190c..51ffe7f0c 100644 --- a/src/Dev/CSVParser.php +++ b/src/Dev/CSVParser.php @@ -31,6 +31,7 @@ use SilverStripe\Control\Director; * $obj->write(); * } * + * @deprecated 4.13.0 Use League\Csv\Reader instead */ class CSVParser implements Iterator { @@ -115,7 +116,7 @@ class CSVParser implements Iterator */ public function __construct($filename, $delimiter = ",", $enclosure = '"') { - Deprecation::notice('5.0', __CLASS__ . ' is deprecated, use ' . Reader::class . ' instead'); + Deprecation::notice('4.13.0', 'Use ' . Reader::class . ' instead', Deprecation::SCOPE_CLASS); $filename = Director::getAbsFile($filename); $this->filename = $filename; $this->delimiter = $delimiter; diff --git a/src/Dev/CsvBulkLoader.php b/src/Dev/CsvBulkLoader.php index a4384078b..2c82a03bd 100644 --- a/src/Dev/CsvBulkLoader.php +++ b/src/Dev/CsvBulkLoader.php @@ -254,11 +254,11 @@ class CsvBulkLoader extends BulkLoader Deprecation::notice('4.12.0', 'Process rows individually instead'); $results = BulkLoader_Result::create(); - $csv = new CSVParser( - $filepath, - $this->delimiter, - $this->enclosure - ); + $delimiter = $this->delimiter; + $enclosure = $this->enclosure; + $csv = Deprecation::withNoReplacement(function () use ($filepath, $delimiter, $enclosure) { + return new CSVParser($filepath, $delimiter, $enclosure); + }); // ColumnMap has two uses, depending on whether hasHeaderRow is set if ($this->columnMap) { diff --git a/src/Dev/TestSession_STResponseWrapper.php b/src/Dev/TestSession_STResponseWrapper.php index 402080e84..6a3407314 100644 --- a/src/Dev/TestSession_STResponseWrapper.php +++ b/src/Dev/TestSession_STResponseWrapper.php @@ -6,6 +6,7 @@ use SilverStripe\Control\HTTPResponse; /** * Wrapper around HTTPResponse to make it look like a SimpleHTTPResposne + * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ class TestSession_STResponseWrapper { @@ -17,6 +18,13 @@ class TestSession_STResponseWrapper public function __construct(HTTPResponse $response) { + Deprecation::withNoReplacement(function () { + Deprecation::notice( + '4.13.0', + 'Will be removed without equivalent functionality to replace it', + Deprecation::SCOPE_CLASS + ); + }); $this->response = $response; } diff --git a/tests/php/Dev/CSVParserTest.php b/tests/php/Dev/CSVParserTest.php index 1ff17fe16..921f9be52 100644 --- a/tests/php/Dev/CSVParserTest.php +++ b/tests/php/Dev/CSVParserTest.php @@ -3,6 +3,7 @@ namespace SilverStripe\Dev\Tests; use SilverStripe\Dev\CSVParser; +use SilverStripe\Dev\Deprecation; use SilverStripe\Dev\SapphireTest; class CSVParserTest extends SapphireTest @@ -19,6 +20,9 @@ class CSVParserTest extends SapphireTest { parent::setUp(); $this->csvPath = __DIR__ . '/CsvBulkLoaderTest/csv/'; + if (Deprecation::isEnabled()) { + $this->markTestSkipped('Test calls deprecated code'); + } } public function testParsingWithHeaders() From 7bc4c9dbc319f4c3d5aacad2468682e9eaaa32fb Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Fri, 17 Feb 2023 18:47:20 +1300 Subject: [PATCH 05/20] MNT Tweak some test to account for slightly different sorting logic in PostgreSQL --- tests/php/ORM/DataListTest.php | 31 ++++++++++++++++++++++++++----- tests/php/Security/MemberTest.php | 2 +- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/tests/php/ORM/DataListTest.php b/tests/php/ORM/DataListTest.php index a50bb6598..99af9d765 100755 --- a/tests/php/ORM/DataListTest.php +++ b/tests/php/ORM/DataListTest.php @@ -1967,16 +1967,14 @@ class DataListTest extends SapphireTest $this->assertSame(['Subteam 1'], Team::get()->limit(1)->column('Title')); $list = Team::get()->sort('Title DESC'); $this->assertSame(['Team 3'], $list->limit(1)->column('Title')); - if ($type !== 'wipes-existing') { - $this->expectException(InvalidArgumentException::class); - } + $this->expectException(InvalidArgumentException::class); if ($type === 'invalid-scalar') { $this->expectExceptionMessage('sort() arguments must either be a string, an array, or null'); } if ($type === 'empty-scalar') { $this->expectExceptionMessage('Invalid sort parameter'); } - // $type === 'wipes-existing' is valid + $list = $list->sort($emtpyValue); $this->assertSame(['Subteam 1'], $list->limit(1)->column('Title')); } @@ -1984,7 +1982,6 @@ class DataListTest extends SapphireTest public function provideSortScalarValues(): array { return [ - [null, 'wipes-existing'], ['', 'empty-scalar'], [[], 'empty-scalar'], [false, 'invalid-scalar'], @@ -1994,6 +1991,30 @@ class DataListTest extends SapphireTest ]; } + /** + * Test passing scalar values to sort() + * + * Explicity tests that sort(null) will wipe any existing sort on a DataList + * + */ + public function testSortNull(): void + { + $list = Team::get()->sort('Title DESC'); + $query = $list->dataQuery()->getFinalisedQuery(); + $this->assertSame( + ['"DataObjectTest_Team"."Title"' => 'DESC'], + $query->getOrderBy(), + 'Calling sort on a DataList sets an Orderby on the underlying query.' + ); + + $list = $list->sort(null); + $query = $list->dataQuery()->getFinalisedQuery(); + $this->assertEmpty( + $query->getOrderBy(), + 'Calling sort with null on a DataList unsets the orderby on the underlying query.' + ); + } + public function testShuffle() { $list = Team::get()->shuffle(); diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php index 703959946..a600190c0 100644 --- a/tests/php/Security/MemberTest.php +++ b/tests/php/Security/MemberTest.php @@ -1831,7 +1831,7 @@ class MemberTest extends FunctionalTest $result = Member::mapInCMSGroups($groups); $this->assertInstanceOf(Map::class, $result); - $this->assertSame($expectedUsers, $result->keys()); + $this->assertEqualsCanonicalizing($expectedUsers, $result->keys()); } public function provideMapInCMSGroups() From 8c396eb1f6178ad7ef2af5e287a603fc0d75ef23 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Fri, 17 Feb 2023 18:53:42 +1300 Subject: [PATCH 06/20] MNT Remove bad PHPDoc comments on test --- tests/php/ORM/DataListTest.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/php/ORM/DataListTest.php b/tests/php/ORM/DataListTest.php index 99af9d765..294123284 100755 --- a/tests/php/ORM/DataListTest.php +++ b/tests/php/ORM/DataListTest.php @@ -1958,8 +1958,6 @@ class DataListTest extends SapphireTest /** * Test passing scalar values to sort() * - * Explicity tests that sort(null) will wipe any existing sort on a DataList - * * @dataProvider provideSortScalarValues */ public function testSortScalarValues(mixed $emtpyValue, string $type): void @@ -1992,10 +1990,7 @@ class DataListTest extends SapphireTest } /** - * Test passing scalar values to sort() - * * Explicity tests that sort(null) will wipe any existing sort on a DataList - * */ public function testSortNull(): void { From e455aa5c5ef1aed8dda9f1f3fde8e829563e60e8 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Mon, 20 Feb 2023 14:37:48 +1300 Subject: [PATCH 07/20] FIX Handle null returns in GridFieldDetailForm_ItemRequest::getNumPages() --- .../GridFieldDetailForm_ItemRequest.php | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php b/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php index fef944f0c..6b9531952 100644 --- a/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php +++ b/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php @@ -665,11 +665,18 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler */ private function getNumPages(GridField $gridField): int { - return $gridField - ->getConfig() - ->getComponentByType(GridFieldPaginator::class) - ->getTemplateParameters($gridField) - ->toMap()['NumPages']; + /** @var GridFieldPaginator $component */ + $component = $gridField + ->getConfig() + ->getComponentByType(GridFieldPaginator::class); + if (is_null($component)) { + return 1; + } + $params = $component->getTemplateParameters($gridField); + if (is_null($params)) { + return 1; + } + return $params->toMap()['NumPages']; } /** From 725497a485bd0ac345a580ea9eeeda812143ecfd Mon Sep 17 00:00:00 2001 From: Michal Kleiner Date: Mon, 20 Feb 2023 22:40:16 +1300 Subject: [PATCH 08/20] MNT Use null safe operator and default to 1 page --- .../GridFieldDetailForm_ItemRequest.php | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php b/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php index 6b9531952..1191cc99d 100644 --- a/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php +++ b/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php @@ -665,18 +665,11 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler */ private function getNumPages(GridField $gridField): int { - /** @var GridFieldPaginator $component */ - $component = $gridField - ->getConfig() - ->getComponentByType(GridFieldPaginator::class); - if (is_null($component)) { - return 1; - } - $params = $component->getTemplateParameters($gridField); - if (is_null($params)) { - return 1; - } - return $params->toMap()['NumPages']; + return $gridField + ->getConfig() + ->getComponentByType(GridFieldPaginator::class) + ?->getTemplateParameters($gridField) + ?->toMap()['NumPages'] ?? 1; } /** From 97f7be502fa48ad27e27fdd5dc7b34e1e1dbb914 Mon Sep 17 00:00:00 2001 From: Loz Calver Date: Fri, 4 Nov 2022 11:59:52 +0000 Subject: [PATCH 09/20] NEW: Add extension hook for field-specific validation --- src/Forms/CompositeField.php | 2 +- src/Forms/ConfirmedPasswordField.php | 18 +-- src/Forms/CurrencyField.php | 6 +- src/Forms/DateField.php | 10 +- src/Forms/DatetimeField.php | 10 +- src/Forms/EmailField.php | 5 +- src/Forms/FileField.php | 7 +- src/Forms/FormField.php | 17 ++- src/Forms/LookupField.php | 2 +- src/Forms/MoneyField.php | 5 +- src/Forms/MultiSelectField.php | 28 ++-- src/Forms/NumericField.php | 23 ++-- src/Forms/OptionsetField.php | 2 +- src/Forms/SingleLookupField.php | 2 +- src/Forms/SingleSelectField.php | 6 +- src/Forms/TextField.php | 5 +- src/Forms/TimeField.php | 9 +- tests/php/Forms/FormFieldTest.php | 120 ++++++++++++++++++ .../FieldValidationExtension.php | 38 ++++++ 19 files changed, 248 insertions(+), 67 deletions(-) create mode 100644 tests/php/Forms/FormFieldTest/FieldValidationExtension.php diff --git a/src/Forms/CompositeField.php b/src/Forms/CompositeField.php index b59a5f118..0e2c4b35a 100644 --- a/src/Forms/CompositeField.php +++ b/src/Forms/CompositeField.php @@ -545,6 +545,6 @@ class CompositeField extends FormField /** @var FormField $child */ $valid = ($child && $child->validate($validator) && $valid); } - return $valid; + return $this->extendValidationResult($valid, $validator); } } diff --git a/src/Forms/ConfirmedPasswordField.php b/src/Forms/ConfirmedPasswordField.php index 479c89d59..ec5fabe22 100644 --- a/src/Forms/ConfirmedPasswordField.php +++ b/src/Forms/ConfirmedPasswordField.php @@ -416,7 +416,7 @@ class ConfirmedPasswordField extends FormField // if field isn't visible, don't validate if (!$this->isSaveable()) { - return true; + return $this->extendValidationResult(true, $validator); } $this->getPasswordField()->setValue($this->value); @@ -431,7 +431,7 @@ class ConfirmedPasswordField extends FormField "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } if (!$this->canBeEmpty) { @@ -443,7 +443,7 @@ class ConfirmedPasswordField extends FormField "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } } @@ -483,7 +483,7 @@ class ConfirmedPasswordField extends FormField "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } } @@ -498,7 +498,7 @@ class ConfirmedPasswordField extends FormField "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } } @@ -513,7 +513,7 @@ class ConfirmedPasswordField extends FormField ), "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } // Check this password is valid for the current user @@ -527,7 +527,7 @@ class ConfirmedPasswordField extends FormField ), "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } // With a valid user and password, check the password is correct @@ -543,12 +543,12 @@ class ConfirmedPasswordField extends FormField ), "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } } } - return true; + return $this->extendValidationResult(true, $validator); } /** diff --git a/src/Forms/CurrencyField.php b/src/Forms/CurrencyField.php index f3358d861..c340fce7b 100644 --- a/src/Forms/CurrencyField.php +++ b/src/Forms/CurrencyField.php @@ -58,6 +58,7 @@ class CurrencyField extends TextField public function validate($validator) { + $result = true; $currencySymbol = preg_quote(DBCurrency::config()->uninherited('currency_symbol') ?? ''); $regex = '/^\s*(\-?' . $currencySymbol . '?|' . $currencySymbol . '\-?)?(\d{1,3}(\,\d{3})*|(\d+))(\.\d{2})?\s*$/'; if (!empty($this->value) && !preg_match($regex ?? '', $this->value ?? '')) { @@ -66,9 +67,10 @@ class CurrencyField extends TextField _t('SilverStripe\\Forms\\Form.VALIDCURRENCY', "Please enter a valid currency"), "validation" ); - return false; + $result = false; } - return true; + + return $this->extendValidationResult($result, $validator); } public function getSchemaValidation() diff --git a/src/Forms/DateField.php b/src/Forms/DateField.php index acb6d536c..880fa0827 100644 --- a/src/Forms/DateField.php +++ b/src/Forms/DateField.php @@ -369,7 +369,7 @@ class DateField extends TextField { // Don't validate empty fields if (empty($this->rawValue)) { - return true; + return $this->extendValidationResult(true, $validator); } // We submitted a value, but it couldn't be parsed @@ -382,7 +382,7 @@ class DateField extends TextField ['format' => $this->getDateFormat()] ) ); - return false; + return $this->extendValidationResult(false, $validator); } // Check min date @@ -406,7 +406,7 @@ class DateField extends TextField ValidationResult::TYPE_ERROR, ValidationResult::CAST_HTML ); - return false; + return $this->extendValidationResult(false, $validator); } } @@ -431,11 +431,11 @@ class DateField extends TextField ValidationResult::TYPE_ERROR, ValidationResult::CAST_HTML ); - return false; + return $this->extendValidationResult(false, $validator); } } - return true; + return $this->extendValidationResult(true, $validator); } /** diff --git a/src/Forms/DatetimeField.php b/src/Forms/DatetimeField.php index a455803d8..849a0ecb8 100644 --- a/src/Forms/DatetimeField.php +++ b/src/Forms/DatetimeField.php @@ -565,7 +565,7 @@ class DatetimeField extends TextField { // Don't validate empty fields if (empty($this->rawValue)) { - return true; + return $this->extendValidationResult(true, $validator); } // We submitted a value, but it couldn't be parsed @@ -578,7 +578,7 @@ class DatetimeField extends TextField ['format' => $this->getDatetimeFormat()] ) ); - return false; + return $this->extendValidationResult(false, $validator); } // Check min date (in server timezone) @@ -602,7 +602,7 @@ class DatetimeField extends TextField ValidationResult::TYPE_ERROR, ValidationResult::CAST_HTML ); - return false; + return $this->extendValidationResult(false, $validator); } } @@ -627,11 +627,11 @@ class DatetimeField extends TextField ValidationResult::TYPE_ERROR, ValidationResult::CAST_HTML ); - return false; + return $this->extendValidationResult(false, $validator); } } - return true; + return $this->extendValidationResult(true, $validator); } public function performReadonlyTransformation() diff --git a/src/Forms/EmailField.php b/src/Forms/EmailField.php index 535173704..68ea66d41 100644 --- a/src/Forms/EmailField.php +++ b/src/Forms/EmailField.php @@ -29,6 +29,7 @@ class EmailField extends TextField */ public function validate($validator) { + $result = true; $this->value = trim($this->value ?? ''); $pattern = '^[a-z0-9!#$%&\'*+/=?^_`{|}~-]+(?:\\.[a-z0-9!#$%&\'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$'; @@ -43,10 +44,10 @@ class EmailField extends TextField 'validation' ); - return false; + $result = false; } - return true; + return $this->extendValidationResult($result, $validator); } public function getSchemaValidation() diff --git a/src/Forms/FileField.php b/src/Forms/FileField.php index 7d087c67f..66b0fe51f 100644 --- a/src/Forms/FileField.php +++ b/src/Forms/FileField.php @@ -187,7 +187,7 @@ class FileField extends FormField implements FileHandleField $fieldName = preg_replace('#\[(.*?)\]$#', '', $this->name ?? ''); if (!isset($_FILES[$fieldName])) { - return true; + return $this->extendValidationResult(true, $validator); } if ($isMultiFileUpload) { @@ -204,11 +204,12 @@ class FileField extends FormField implements FileHandleField $isValid = false; } } - return $isValid; + return $this->extendValidationResult($isValid, $validator); } // regular single-file upload - return $this->validateFileData($validator, $_FILES[$this->name]); + $result = $this->validateFileData($validator, $_FILES[$this->name]); + return $this->extendValidationResult($result, $validator); } /** diff --git a/src/Forms/FormField.php b/src/Forms/FormField.php index 8574185ec..763d388b8 100644 --- a/src/Forms/FormField.php +++ b/src/Forms/FormField.php @@ -1219,18 +1219,29 @@ class FormField extends RequestHandler return strtolower(preg_replace('/Field$/', '', $type->getShortName() ?? '') ?? ''); } + /** + * Utility method to call an extension hook which allows the result of validate() calls to be adjusted + * + * @param bool $result + * @param Validator $validator + * @return bool + */ + protected function extendValidationResult(bool $result, Validator $validator): bool + { + $this->extend('updateValidationResult', $result, $validator); + return $result; + } + /** * Abstract method each {@link FormField} subclass must implement, determines whether the field * is valid or not based on the value. * - * @todo Make this abstract. - * * @param Validator $validator * @return bool */ public function validate($validator) { - return true; + return $this->extendValidationResult(true, $validator); } /** diff --git a/src/Forms/LookupField.php b/src/Forms/LookupField.php index 6a5c31ba5..113ba5538 100644 --- a/src/Forms/LookupField.php +++ b/src/Forms/LookupField.php @@ -73,7 +73,7 @@ class LookupField extends MultiSelectField */ public function validate($validator) { - return true; + return $this->extendValidationResult(true, $validator); } /** diff --git a/src/Forms/MoneyField.php b/src/Forms/MoneyField.php index f6da4d8dc..6f1c231a8 100644 --- a/src/Forms/MoneyField.php +++ b/src/Forms/MoneyField.php @@ -334,11 +334,12 @@ class MoneyField extends FormField ['currency' => $currency] ) ); - return false; + return $this->extendValidationResult(false, $validator); } // Field-specific validation - return $this->fieldAmount->validate($validator) && $this->fieldCurrency->validate($validator); + $result = $this->fieldAmount->validate($validator) && $this->fieldCurrency->validate($validator); + return $this->extendValidationResult($result, $validator); } public function setForm($form) diff --git a/src/Forms/MultiSelectField.php b/src/Forms/MultiSelectField.php index 4c3d1e8dd..7fe0a9b59 100644 --- a/src/Forms/MultiSelectField.php +++ b/src/Forms/MultiSelectField.php @@ -247,21 +247,23 @@ abstract class MultiSelectField extends SelectField return true; } ); - if (empty($invalidValues)) { - return true; + + $result = true; + if (!empty($invalidValues)) { + $result = false; + // List invalid items + $validator->validationError( + $this->getName(), + _t( + 'SilverStripe\\Forms\\MultiSelectField.SOURCE_VALIDATION', + "Please select values within the list provided. Invalid option(s) {value} given", + ['value' => implode(',', $invalidValues)] + ), + "validation" + ); } - // List invalid items - $validator->validationError( - $this->getName(), - _t( - 'SilverStripe\\Forms\\MultiSelectField.SOURCE_VALIDATION', - "Please select values within the list provided. Invalid option(s) {value} given", - ['value' => implode(',', $invalidValues)] - ), - "validation" - ); - return false; + return $this->extendValidationResult($result, $validator); } /** diff --git a/src/Forms/NumericField.php b/src/Forms/NumericField.php index 4c05caa46..776ea381f 100644 --- a/src/Forms/NumericField.php +++ b/src/Forms/NumericField.php @@ -191,20 +191,21 @@ class NumericField extends TextField */ public function validate($validator) { + $result = true; // false signifies invalid value due to failed parse() - if ($this->value !== false) { - return true; + if ($this->value === false) { + $validator->validationError( + $this->name, + _t( + 'SilverStripe\\Forms\\NumericField.VALIDATION', + "'{value}' is not a number, only numbers can be accepted for this field", + ['value' => $this->originalValue] + ) + ); + $result = false; } - $validator->validationError( - $this->name, - _t( - 'SilverStripe\\Forms\\NumericField.VALIDATION', - "'{value}' is not a number, only numbers can be accepted for this field", - ['value' => $this->originalValue] - ) - ); - return false; + return $this->extendValidationResult($result, $validator); } public function getSchemaValidation() diff --git a/src/Forms/OptionsetField.php b/src/Forms/OptionsetField.php index f07d6b08e..f47d6dd57 100644 --- a/src/Forms/OptionsetField.php +++ b/src/Forms/OptionsetField.php @@ -140,7 +140,7 @@ class OptionsetField extends SingleSelectField public function validate($validator) { if (!$this->Value()) { - return true; + return $this->extendValidationResult(true, $validator); } return parent::validate($validator); diff --git a/src/Forms/SingleLookupField.php b/src/Forms/SingleLookupField.php index f0ce89c3b..9f3e8883a 100644 --- a/src/Forms/SingleLookupField.php +++ b/src/Forms/SingleLookupField.php @@ -44,7 +44,7 @@ class SingleLookupField extends SingleSelectField */ public function validate($validator) { - return true; + return $this->extendValidationResult(true, $validator); } /** diff --git a/src/Forms/SingleSelectField.php b/src/Forms/SingleSelectField.php index 8f847cb59..2230cd807 100644 --- a/src/Forms/SingleSelectField.php +++ b/src/Forms/SingleSelectField.php @@ -137,13 +137,13 @@ abstract class SingleSelectField extends SelectField // Use selection rules to check which are valid foreach ($validValues as $formValue) { if ($this->isSelectedValue($formValue, $selected)) { - return true; + return $this->extendValidationResult(true, $validator); } } } else { if ($this->getHasEmptyDefault() || !$validValues || in_array('', $validValues ?? [])) { // Check empty value - return true; + return $this->extendValidationResult(true, $validator); } $selected = '(none)'; } @@ -158,7 +158,7 @@ abstract class SingleSelectField extends SelectField ), "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } public function castedCopy($classOrCopy) diff --git a/src/Forms/TextField.php b/src/Forms/TextField.php index 7988f6196..4247afa6b 100644 --- a/src/Forms/TextField.php +++ b/src/Forms/TextField.php @@ -125,6 +125,7 @@ class TextField extends FormField implements TippableFieldInterface */ public function validate($validator) { + $result = true; if (!is_null($this->maxLength) && mb_strlen($this->value ?? '') > $this->maxLength) { $name = strip_tags($this->Title() ? $this->Title() : $this->getName()); $validator->validationError( @@ -136,9 +137,9 @@ class TextField extends FormField implements TippableFieldInterface ), "validation" ); - return false; + $result = false; } - return true; + return $this->extendValidationResult($result, $validator); } public function getSchemaValidation() diff --git a/src/Forms/TimeField.php b/src/Forms/TimeField.php index 43a796e1a..525f6dfc4 100644 --- a/src/Forms/TimeField.php +++ b/src/Forms/TimeField.php @@ -324,9 +324,10 @@ class TimeField extends TextField { // Don't validate empty fields if (empty($this->rawValue)) { - return true; + return $this->extendValidationResult(true, $validator); } + $result = true; // We submitted a value, but it couldn't be parsed if (empty($this->value)) { $validator->validationError( @@ -337,9 +338,11 @@ class TimeField extends TextField ['format' => $this->getTimeFormat()] ) ); - return false; + $result = false; } - return true; + + $this->extendValidationResult($result, $validator); + return $result; } /** diff --git a/tests/php/Forms/FormFieldTest.php b/tests/php/Forms/FormFieldTest.php index 47a3ff946..026a58837 100644 --- a/tests/php/Forms/FormFieldTest.php +++ b/tests/php/Forms/FormFieldTest.php @@ -2,21 +2,40 @@ namespace SilverStripe\Forms\Tests; +use Exception; use LogicException; use ReflectionClass; use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Config\Config; use SilverStripe\Dev\SapphireTest; +use SilverStripe\Dev\TestOnly; use SilverStripe\Forms\CompositeField; +use SilverStripe\Forms\FieldGroup; use SilverStripe\Forms\FieldList; use SilverStripe\Forms\Form; use SilverStripe\Forms\FormField; +use SilverStripe\Forms\GridField\GridField; +use SilverStripe\Forms\GridField\GridField_FormAction; +use SilverStripe\Forms\GridField\GridState; use SilverStripe\Forms\NullableField; +use SilverStripe\Forms\PopoverField; +use SilverStripe\Forms\PrintableTransformation_TabSet; use SilverStripe\Forms\RequiredFields; +use SilverStripe\Forms\SelectionGroup; +use SilverStripe\Forms\SelectionGroup_Item; +use SilverStripe\Forms\Tab; +use SilverStripe\Forms\Tests\FormFieldTest\FieldValidationExtension; use SilverStripe\Forms\Tests\FormFieldTest\TestExtension; use SilverStripe\Forms\TextField; use SilverStripe\Forms\Tip; +use SilverStripe\Forms\ToggleCompositeField; +use SilverStripe\Forms\TreeDropdownField; +use SilverStripe\Forms\TreeDropdownField_Readonly; use SilverStripe\ORM\ValidationResult; +use SilverStripe\Security\Group; +use SilverStripe\Security\Permission; +use SilverStripe\Security\PermissionCheckboxSetField; +use SilverStripe\Security\PermissionCheckboxSetField_Readonly; class FormFieldTest extends SapphireTest { @@ -24,6 +43,7 @@ class FormFieldTest extends SapphireTest protected static $required_extensions = [ FormField::class => [ TestExtension::class, + FieldValidationExtension::class, ], ]; @@ -455,6 +475,106 @@ class FormFieldTest extends SapphireTest ); } + public function testValidationExtensionHooks() + { + /** @var TextField|FieldValidationExtension $field */ + $field = new TextField('Test'); + $field->setMaxLength(5); + $field->setValue('IAmLongerThan5Characters'); + $result = $field->validate(new RequiredFields('Test')); + $this->assertFalse($result); + + // Call extension method in FieldValidationExtension + $field->setExcludeFromValidation(true); + $result = $field->validate(new RequiredFields('Test')); + $this->assertTrue($result); + + // Call extension methods in FieldValidationExtension + $field->setValue('1234'); + $field->setExcludeFromValidation(false); + $field->setTriggerTestValidationError(true); + + // Ensure messages set via updateValidationResult() propagate through to form fields after validation + $form = new Form(null, 'TestForm', new FieldList($field), new FieldList(), new RequiredFields()); + $form->validationResult(); + $schema = $field->getSchemaState(); + $this->assertEquals( + 'A test error message', + $schema['message']['value'] + ); + } + + public function testValidationExtensionHooksAreCalledOnFormFieldSubclasses() + { + // Can't use a dataProvider for this as dataProviders are fetched very early by phpunit, + // and the ClassManifest isn't ready then + $formFieldClasses = ClassInfo::subclassesFor(FormField::class, false); + foreach ($formFieldClasses as $formFieldClass) { + $reflection = new ReflectionClass($formFieldClass); + // Skip abstract classes, like MultiSelectField, and fields that only exist for unit tests + if ($reflection->isAbstract() || is_a($formFieldClass, TestOnly::class, true)) { + continue; + } + + switch ($formFieldClass) { + case NullableField::class: + case CompositeField::class: + case FieldGroup::class: + case PopoverField::class: + $args = [TextField::create('Test2')]; + break; + case SelectionGroup_Item::class: + $args = ['Test', [TextField::create('Test2')]]; + break; + case ToggleCompositeField::class: + $args = ['Test', 'Test', TextField::create('Test2')]; + break; + case PrintableTransformation_TabSet::class: + $args = [Tab::create('TestTab', 'Testtab', TextField::create('Test2'))]; + break; + case TreeDropdownField::class: + case TreeDropdownField_Readonly::class: + $args = ['Test', 'Test', Group::class]; + break; + case PermissionCheckboxSetField::class: + case PermissionCheckboxSetField_Readonly::class: + $args = ['Test', 'Test', Permission::class, 'Test']; + break; + case SelectionGroup::class: + $args = ['Test', []]; + break; + case GridField_FormAction::class: + $args = [GridField::create('GF'), 'Test', 'Test label', 'Test action name', []]; + break; + case GridState::class: + $args = [GridField::create('GF')]; + break; + default: + $args = ['Test', 'Test label']; + } + + // Assert that extendValidationResult is called once each time ->validate() is called + $mock = $this->getMockBuilder($formFieldClass) + ->setConstructorArgs($args) + ->onlyMethods(['extendValidationResult']) + ->getMock(); + $mock->expects($invocationRule = $this->once()) + ->method('extendValidationResult') + ->will($this->returnValue(true)); + + $isValid = $mock->validate(new RequiredFields()); + $this->assertTrue($isValid, "$formFieldClass should be valid"); + + // This block is not essential and only exists to make test debugging easier - without this, + // the error message on failure is generic and doesn't include the class name that failed + try { + $invocationRule->verify(); + } catch (Exception $e) { + $this->fail("Expectation failed for '$formFieldClass' class: {$e->getMessage()}"); + } + } + } + public function testHasClass() { $field = new FormField('Test'); diff --git a/tests/php/Forms/FormFieldTest/FieldValidationExtension.php b/tests/php/Forms/FormFieldTest/FieldValidationExtension.php new file mode 100644 index 000000000..8376dc4af --- /dev/null +++ b/tests/php/Forms/FormFieldTest/FieldValidationExtension.php @@ -0,0 +1,38 @@ +excludeFromValidation) { + $result = true; + return; + } + + if ($this->triggerTestValidationError) { + $result = false; + $validator->validationError($this->owner->getName(), 'A test error message'); + return; + } + } + + public function setExcludeFromValidation(bool $exclude) + { + $this->excludeFromValidation = $exclude; + } + + public function setTriggerTestValidationError(bool $triggerTestValidationError) + { + $this->triggerTestValidationError = $triggerTestValidationError; + } +} From 0075bf6e49973ac357a56a55d1053b86497fe98b Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Wed, 22 Feb 2023 09:40:27 +1300 Subject: [PATCH 10/20] NEW Access dynamic data inside ViewableData --- src/ORM/FieldType/DBComposite.php | 9 +++++---- src/View/ViewableData.php | 23 +++++++++++++++++++---- tests/php/ORM/DBCompositeTest.php | 12 ++++++++++++ tests/php/View/ViewableDataTest.php | 11 +++++++++++ 4 files changed, 47 insertions(+), 8 deletions(-) diff --git a/src/ORM/FieldType/DBComposite.php b/src/ORM/FieldType/DBComposite.php index 05423ce08..a4d936b2e 100644 --- a/src/ORM/FieldType/DBComposite.php +++ b/src/ORM/FieldType/DBComposite.php @@ -2,6 +2,7 @@ namespace SilverStripe\ORM\FieldType; +use InvalidArgumentException; use SilverStripe\Core\Injector\Injector; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DB; @@ -270,11 +271,11 @@ abstract class DBComposite extends DBField { $this->objCacheClear(); - // Non-db fields get assigned as normal properties if (!$this->hasField($field)) { - parent::setField($field, $value); - - return $this; + throw new InvalidArgumentException(implode(' ', [ + "Field $field does not exist.", + 'If this was accessed via a dynamic property then call setDynamicData() instead.' + ])); } // Set changed diff --git a/src/View/ViewableData.php b/src/View/ViewableData.php index 77d25ae4f..7c3d3f814 100644 --- a/src/View/ViewableData.php +++ b/src/View/ViewableData.php @@ -73,7 +73,7 @@ class ViewableData implements IteratorAggregate /** * Acts as a PHP 8.2+ compliant replacement for dynamic properties */ - private array $data = []; + private array $dynamicData = []; // ----------------------------------------------------------------------------------------------------------------- @@ -203,7 +203,7 @@ class ViewableData implements IteratorAggregate */ public function hasField($field) { - return property_exists($this, $field) || isset($this->data[$field]); + return property_exists($this, $field) || $this->hasDynamicData($field); } /** @@ -217,7 +217,7 @@ class ViewableData implements IteratorAggregate if ($this->isAccessibleProperty($field)) { return $this->$field; } - return $this->data[$field]; + return $this->getDynamicData($field); } /** @@ -236,10 +236,25 @@ class ViewableData implements IteratorAggregate if ($this->isAccessibleProperty($field)) { $this->$field = $value; } - $this->data[$field] = $value; + return $this->setDynamicData($field, $value); + } + + public function getDynamicData(string $field): mixed + { + return $this->hasDynamicData($field) ? $this->dynamicData[$field] : null; + } + + public function setDynamicData(string $field, mixed $value): static + { + $this->dynamicData[$field] = $value; return $this; } + public function hasDynamicData(string $field): bool + { + return array_key_exists($field, $this->dynamicData); + } + /** * Returns true if a method exists for the current class which isn't private. * Also returns true for private methods if $this is ViewableData (not a subclass) diff --git a/tests/php/ORM/DBCompositeTest.php b/tests/php/ORM/DBCompositeTest.php index cbad5910c..4afd93ff0 100644 --- a/tests/php/ORM/DBCompositeTest.php +++ b/tests/php/ORM/DBCompositeTest.php @@ -5,6 +5,7 @@ namespace SilverStripe\ORM\Tests; use SilverStripe\ORM\FieldType\DBMoney; use SilverStripe\ORM\DataObject; use SilverStripe\Dev\SapphireTest; +use InvalidArgumentException; class DBCompositeTest extends SapphireTest { @@ -108,4 +109,15 @@ class DBCompositeTest extends SapphireTest $this->assertEquals('DBCompositeTest_SubclassedDBFieldObject', $object2->dbObject('OtherMoney')->getTable()); $this->assertEquals('DBCompositeTest_SubclassedDBFieldObject', $object2->dbObject('OverriddenMoney')->getTable()); } + + public function testSetFieldDynamicPropertyException() + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage(implode(' ', [ + 'Field abc does not exist.', + 'If this was accessed via a dynamic property then call setDynamicData() instead.' + ])); + $object = new DBCompositeTest\TestObject(); + $object->MyMoney->abc = 'def'; + } } diff --git a/tests/php/View/ViewableDataTest.php b/tests/php/View/ViewableDataTest.php index 5ddfb0399..15ff74252 100644 --- a/tests/php/View/ViewableDataTest.php +++ b/tests/php/View/ViewableDataTest.php @@ -267,4 +267,15 @@ class ViewableDataTest extends SapphireTest $output = $reflectionMethod->invokeArgs(new ViewableData(), ['objCache']); $this->assertTrue($output, 'Property should be accessible'); } + + public function testDynamicData() + { + $obj = (object) ['SomeField' => [1, 2, 3]]; + $viewableData = new ViewableData(); + $this->assertFalse($viewableData->hasDynamicData('abc')); + $viewableData->setDynamicData('abc', $obj); + $this->assertTrue($viewableData->hasDynamicData('abc')); + $this->assertSame($obj, $viewableData->getDynamicData('abc')); + $this->assertSame($obj, $viewableData->abc); + } } From 0633f2ed0d298985927805685be81bb6ae8d305e Mon Sep 17 00:00:00 2001 From: Loz Calver Date: Wed, 22 Feb 2023 16:09:35 +0000 Subject: [PATCH 11/20] Add kitchensink fields to formfield validation test --- tests/php/Forms/FormFieldTest.php | 40 +++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/php/Forms/FormFieldTest.php b/tests/php/Forms/FormFieldTest.php index 026a58837..70bf0c515 100644 --- a/tests/php/Forms/FormFieldTest.php +++ b/tests/php/Forms/FormFieldTest.php @@ -516,7 +516,12 @@ class FormFieldTest extends SapphireTest continue; } + // Create appropriate constructor arguments for the form field class. These don't have to be offer realistic + // data, they just need to ensure we can construct the field and call ->validate() on it switch ($formFieldClass) { + // + // Fields in framework with specific argument requirements + // case NullableField::class: case CompositeField::class: case FieldGroup::class: @@ -549,6 +554,41 @@ class FormFieldTest extends SapphireTest case GridState::class: $args = [GridField::create('GF')]; break; + // + // Fields from other modules included in the kitchensink recipe + // + case \SilverStripe\Blog\Admin\GridFieldFormAction::class: + $args = [GridField::create('GF'), 'Test', 'Test label', 'Test action name', []]; + break; + case \SilverStripe\Blog\Forms\BlogAdminSidebar::class: + $args = [TextField::create('Test2')]; + break; + case \SilverStripe\CKANRegistry\Forms\PresentedOptionsField::class: + $args = ['Test', \SilverStripe\CKANRegistry\Model\Resource::create()]; + break; + case \SilverStripe\DocumentConverter\SettingsField::class: + $args = []; + break; + case \DNADesign\Elemental\Forms\ElementalAreaField::class: + $args = ['Test', \DNADesign\Elemental\Models\ElementalArea::create(), []]; + break; + case \SilverStripe\MFA\FormField\RegisteredMFAMethodListField::class: + $args = ['Test', 'Test label', 1]; + break; + case \SilverStripe\Subsites\Forms\SubsitesTreeDropdownField::class: + $args = ['Test', 'Test', Group::class]; + break; + case \SilverStripe\UserForms\FormField\UserFormsCompositeField::class: + case \SilverStripe\UserForms\FormField\UserFormsGroupField::class: + case \SilverStripe\UserForms\FormField\UserFormsStepField::class: + $args = [TextField::create('Test2')]; + break; + case \Symbiote\AdvancedWorkflow\FormFields\WorkflowField::class: + $args = ['Test', 'Test label', \Symbiote\AdvancedWorkflow\DataObjects\WorkflowDefinition::create()]; + break; + // + // Default arguments, this covers most simple form fields + // default: $args = ['Test', 'Test label']; } From c82d11ef70b59daeabd386e16cf871bb25e5bb70 Mon Sep 17 00:00:00 2001 From: Chris Penny Date: Thu, 23 Feb 2023 11:08:31 +1300 Subject: [PATCH 12/20] Add isFlushed() to Kernel interface Also add explicit return type --- src/Core/BaseKernel.php | 4 ++-- src/Core/CoreKernel.php | 12 ++---------- src/Core/DatabaselessKernel.php | 11 +++-------- src/Core/Kernel.php | 7 +++++++ 4 files changed, 14 insertions(+), 20 deletions(-) diff --git a/src/Core/BaseKernel.php b/src/Core/BaseKernel.php index a3893ab9d..2b7ffbd2e 100644 --- a/src/Core/BaseKernel.php +++ b/src/Core/BaseKernel.php @@ -188,7 +188,7 @@ abstract class BaseKernel implements Kernel $this->getIncludeTests(), $flush ); - + // Flush config if ($flush) { $config = $this->getConfigLoader()->getManifest(); @@ -265,7 +265,7 @@ abstract class BaseKernel implements Kernel abstract public function boot($flush = false); - abstract public function isFlushed(); + abstract public function isFlushed(): ?bool; /** * Check if there's a legacy _ss_environment.php file diff --git a/src/Core/CoreKernel.php b/src/Core/CoreKernel.php index be3ed1821..3af15bb99 100644 --- a/src/Core/CoreKernel.php +++ b/src/Core/CoreKernel.php @@ -15,11 +15,8 @@ class CoreKernel extends BaseKernel /** * Indicates whether the Kernel has been flushed on boot - * Uninitialised before boot - * - * @var bool */ - private $flush; + private ?bool $flush = null; /** * @param false $flush @@ -217,12 +214,7 @@ class CoreKernel extends BaseKernel return null; } - /** - * Returns whether the Kernel has been flushed on boot - * - * @return bool|null null if the kernel hasn't been booted yet - */ - public function isFlushed() + public function isFlushed(): ?bool { return $this->flush; } diff --git a/src/Core/DatabaselessKernel.php b/src/Core/DatabaselessKernel.php index d5c981ba4..c21ec3681 100644 --- a/src/Core/DatabaselessKernel.php +++ b/src/Core/DatabaselessKernel.php @@ -16,11 +16,9 @@ class DatabaselessKernel extends BaseKernel { /** * Indicates whether the Kernel has been flushed on boot - * Uninitialised before boot - * - * @var bool + * Null before boot */ - private $flush; + private ?bool $flush = null; /** * Allows disabling of the configured error handling. @@ -53,10 +51,7 @@ class DatabaselessKernel extends BaseKernel $this->setBooted(true); } - /** - * @return bool - */ - public function isFlushed() + public function isFlushed(): ?bool { return $this->flush; } diff --git a/src/Core/Kernel.php b/src/Core/Kernel.php index a80a23203..a64590bce 100644 --- a/src/Core/Kernel.php +++ b/src/Core/Kernel.php @@ -132,4 +132,11 @@ interface Kernel * @return $this */ public function setEnvironment($environment); + + /** + * Returns whether the Kernel has been flushed on boot + * + * @return bool|null null if the kernel hasn't been booted yet + */ + public function isFlushed(): ?bool; } From 652281507f2f0c17126806f8aa30f180a0fa6a95 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Mon, 27 Feb 2023 15:25:27 +1300 Subject: [PATCH 13/20] FIX Correctly identify deprecated API in withNoReplacement (#10706) --- src/Dev/Deprecation.php | 12 +++++++++--- tests/php/Dev/DeprecationTest.php | 16 ++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/Dev/Deprecation.php b/src/Dev/Deprecation.php index fff3e8971..871aa0834 100644 --- a/src/Dev/Deprecation.php +++ b/src/Dev/Deprecation.php @@ -177,6 +177,12 @@ class Deprecation $level = 1; } $newLevel = $level; + // handle closures inside withNoReplacement() + if (self::$insideWithNoReplacement + && substr($backtrace[$newLevel]['function'], -strlen('{closure}')) === '{closure}' + ) { + $newLevel = $newLevel + 2; + } // handle call_user_func if ($level === 4 && strpos($backtrace[2]['function'] ?? '', 'call_user_func') !== false) { $newLevel = 5; @@ -277,10 +283,10 @@ class Deprecation // Do not add to self::$userErrorMessageBuffer, as the backtrace is too expensive return; } - + // Getting a backtrace is slow, so we only do it if we need it $backtrace = null; - + // Get the calling scope if ($scope == Deprecation::SCOPE_METHOD) { $backtrace = debug_backtrace(0); @@ -291,7 +297,7 @@ class Deprecation } else { $caller = false; } - + if (substr($string, -1) != '.') { $string .= "."; } diff --git a/tests/php/Dev/DeprecationTest.php b/tests/php/Dev/DeprecationTest.php index fac1ad726..8b0cf8184 100644 --- a/tests/php/Dev/DeprecationTest.php +++ b/tests/php/Dev/DeprecationTest.php @@ -151,6 +151,22 @@ class DeprecationTest extends SapphireTest Deprecation::outputNotices(); } + public function testNoticeWithNoReplacementTrue() + { + $message = implode(' ', [ + 'SilverStripe\Dev\Tests\DeprecationTest->testNoticeWithNoReplacementTrue is deprecated.', + 'My message.', + 'Called from PHPUnit\Framework\TestCase->runTest.' + ]); + $this->expectDeprecation(); + $this->expectDeprecationMessage($message); + Deprecation::enable(true); + Deprecation::withNoReplacement(function () { + Deprecation::notice('123', 'My message.'); + }); + Deprecation::outputNotices(); + } + public function testClassWithNoReplacement() { $message = implode(' ', [ From 6669d54f5928d22ee94e966747069b61aa06d14e Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Mon, 27 Feb 2023 18:13:31 +1300 Subject: [PATCH 14/20] FIX Wrap deprecated config with no replacement (#10704) --- src/Control/Director.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Control/Director.php b/src/Control/Director.php index ca26754d9..fe5c3e379 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -704,7 +704,9 @@ class Director implements TemplateGlobalProvider */ public static function publicDir() { - $alternate = self::config()->uninherited('alternate_public_dir'); + $alternate = Deprecation::withNoReplacement(function () { + return self::config()->uninherited('alternate_public_dir'); + }); if (isset($alternate)) { return $alternate; } From 5295ba6c168b6d7c1a23913e2c99fbe87aba6781 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Wed, 1 Mar 2023 11:36:08 +1300 Subject: [PATCH 15/20] API Throw deprecation warnings for bad configuration (#10702) --- src/Control/Director.php | 1 - src/Control/HTTPApplication.php | 51 +++++++++++++++++++ src/Dev/Deprecation.php | 12 ++--- .../swiftmailer/Swift/MailTransport.php | 7 +++ .../Swift/Transport/MailInvoker.php | 1 + .../Swift/Transport/MailTransport.php | 7 ++- .../Swift/Transport/SimpleMailInvoker.php | 10 ++++ 7 files changed, 78 insertions(+), 11 deletions(-) diff --git a/src/Control/Director.php b/src/Control/Director.php index fe5c3e379..3ac23350e 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -73,7 +73,6 @@ class Director implements TemplateGlobalProvider /** * @config * @var string - * @deprecated 4.13.0 Will be removed without equivalent functionality to replace it */ private static $alternate_base_folder; diff --git a/src/Control/HTTPApplication.php b/src/Control/HTTPApplication.php index aba380796..f1f6fd493 100644 --- a/src/Control/HTTPApplication.php +++ b/src/Control/HTTPApplication.php @@ -6,12 +6,15 @@ use SilverStripe\Control\Middleware\HTTPMiddlewareAware; use SilverStripe\Core\Application; use SilverStripe\Core\Environment; use SilverStripe\Core\Kernel; +use SilverStripe\Core\Manifest\Module; use SilverStripe\Core\Startup\FlushDiscoverer; use SilverStripe\Core\Startup\CompositeFlushDiscoverer; use SilverStripe\Core\Startup\CallbackFlushDiscoverer; use SilverStripe\Core\Startup\RequestFlushDiscoverer; use SilverStripe\Core\Startup\ScheduledFlushDiscoverer; use SilverStripe\Core\Startup\DeployFlushDiscoverer; +use SilverStripe\Dev\Deprecation; +use SilverStripe\GraphQL\TypeCreator; /** * Invokes the HTTP application within an ErrorControlChain @@ -133,6 +136,10 @@ class HTTPApplication implements Application return $this->callMiddleware($request, function ($request) use ($callback, $flush) { // Pre-request boot $this->getKernel()->boot($flush); + + // This is the earliest point we can do this and guarantee it's hit exactly once per request. + $this->warnAboutDeprecatedSetups(); + return call_user_func($callback, $request); }); } catch (HTTPResponse_Exception $ex) { @@ -141,4 +148,48 @@ class HTTPApplication implements Application $this->getKernel()->shutdown(); } } + + /** + * Trigger deprecation notices for legacy configuration which is deprecated but + * doesn't have deprecation notices directly on the relevant API + * + * Don't remove this method even if it's just a no-op - we'll reuse this mechanism + * in the future as needed. + */ + private function warnAboutDeprecatedSetups() + { + // TypeCreator is a class unique to GraphQL v3 - we use it in other areas to detect + // which version is being used. + if (class_exists(TypeCreator::class)) { + Deprecation::notice( + '4.13.0', + 'silverstripe/graphql 3.x is deprecated. Upgrade to 4.x instead.' + . ' See https://docs.silverstripe.org/en/4/upgrading/upgrading_to_graphql_4/', + Deprecation::SCOPE_GLOBAL + ); + } + + // The alternate_public_dir config property is deprecated, but because it's + // always fetched it'll throw a deprecation warning whether you've set it or not. + // There are also multiple mechanisms which can result in this bad configuration. + if (PUBLIC_DIR !== 'public' || Director::publicDir() !== PUBLIC_DIR) { + Deprecation::notice( + '4.13.0', + 'Use of a public webroot other than "public" is deprecated.' + . ' See https://docs.silverstripe.org/en/4/changelogs/4.1.0#public-folder/', + Deprecation::SCOPE_GLOBAL + ); + } + + // This change of defaults has no other deprecation notice being emitted currently. + $project = new Module(BASE_PATH, BASE_PATH); + if ($project->getResourcesDir() === '') { + Deprecation::notice( + '4.13.0', + 'The RESOURCES_DIR constant will change to "_resources" by default.' + . ' See https://docs.silverstripe.org/en/5/changelogs/5.0.0/#api-general', + Deprecation::SCOPE_GLOBAL + ); + } + } } diff --git a/src/Dev/Deprecation.php b/src/Dev/Deprecation.php index 871aa0834..66b39c070 100644 --- a/src/Dev/Deprecation.php +++ b/src/Dev/Deprecation.php @@ -10,19 +10,13 @@ use SilverStripe\Core\Injector\InjectorLoader; use SilverStripe\Core\Manifest\Module; /** - * Handles raising an notice when accessing a deprecated method + * Handles raising an notice when accessing a deprecated method, class, configuration, or behaviour. * - * A pattern used in SilverStripe when deprecating a method is to add something like - * user_error('This method is deprecated', E_USER_NOTICE); - * to the method - * - * However sometimes we want to mark that a method will be deprecated in some future version and shouldn't be used in + * Sometimes we want to mark that a method will be deprecated in some future version and shouldn't be used in * new code, but not forbid in the current version - for instance when that method is still heavily used in framework * or cms. * - * This class abstracts the above pattern and adds a way to do that. - * - * Each call to notice passes a version that the notice will be valid from. + * See https://docs.silverstripe.org/en/contributing/release_process/#deprecation */ class Deprecation { diff --git a/thirdparty/swiftmailer/Swift/MailTransport.php b/thirdparty/swiftmailer/Swift/MailTransport.php index ec3b892a2..a9052972b 100644 --- a/thirdparty/swiftmailer/Swift/MailTransport.php +++ b/thirdparty/swiftmailer/Swift/MailTransport.php @@ -5,6 +5,8 @@ * It has been slightly modified to meet phpcs standards and initialise Swift_DependencyContainer */ +use SilverStripe\Dev\Deprecation; + /* * This file is part of SwiftMailer. * (c) 2004-2009 Chris Corbyn @@ -19,6 +21,7 @@ * @author Chris Corbyn * * at deprecated since 5.4.5 (to be removed in 6.0) + * @deprecated 4.12.0 Sending email using the mail() function is deprecated. Use sendmail or SMTP instead */ // @codingStandardsIgnoreStart // ignore missing namespace @@ -32,6 +35,10 @@ class Swift_MailTransport extends Swift_Transport_MailTransport */ public function __construct($extraParams = '-f%s') { + Deprecation::withNoReplacement(function () { + Deprecation::notice('4.12.0', 'Sending email using the mail() function is deprecated. Use sendmail or SMTP instead', Deprecation::SCOPE_CLASS); + }); + call_user_func_array( [$this, 'Swift_Transport_MailTransport::__construct'], $this->getDependencies() ?? [] diff --git a/thirdparty/swiftmailer/Swift/Transport/MailInvoker.php b/thirdparty/swiftmailer/Swift/Transport/MailInvoker.php index 2a23e24d2..f36af3b85 100644 --- a/thirdparty/swiftmailer/Swift/Transport/MailInvoker.php +++ b/thirdparty/swiftmailer/Swift/Transport/MailInvoker.php @@ -17,6 +17,7 @@ * This interface intercepts calls to the mail() function. * * @author Chris Corbyn + * @deprecated 4.12.0 Will be replaced with symfony/mailer */ // @codingStandardsIgnoreStart // ignore missing namespace diff --git a/thirdparty/swiftmailer/Swift/Transport/MailTransport.php b/thirdparty/swiftmailer/Swift/Transport/MailTransport.php index 84fd03a15..5583c04be 100644 --- a/thirdparty/swiftmailer/Swift/Transport/MailTransport.php +++ b/thirdparty/swiftmailer/Swift/Transport/MailTransport.php @@ -5,6 +5,8 @@ * It has been slightly modified to meet phpcs standards and to update method signatures to match the swiftmailer v6 */ +use SilverStripe\Dev\Deprecation; + /* * This file is part of SwiftMailer. * (c) 2004-2009 Chris Corbyn @@ -26,7 +28,7 @@ * * @author Chris Corbyn * - * at deprecated since 5.4.5 (to be removed in 6.0) + * @deprecated 4.12.0 Sending email using the mail() function is deprecated. Use sendmail or SMTP instead */ // @codingStandardsIgnoreStart // ignore missing namespace @@ -50,6 +52,9 @@ class Swift_Transport_MailTransport implements Swift_Transport */ public function __construct(Swift_Transport_MailInvoker $invoker, Swift_Events_EventDispatcher $eventDispatcher) { + Deprecation::withNoReplacement(function () { + Deprecation::notice('4.12.0', 'Sending email using the mail() function is deprecated. Use sendmail or SMTP instead', Deprecation::SCOPE_CLASS); + }); // @trigger_error(sprintf('The %s class is deprecated since version 5.4.5 and will be removed in 6.0. Use the Sendmail or SMTP transport instead.', __CLASS__), E_USER_DEPRECATED); $this->_invoker = $invoker; diff --git a/thirdparty/swiftmailer/Swift/Transport/SimpleMailInvoker.php b/thirdparty/swiftmailer/Swift/Transport/SimpleMailInvoker.php index 8094e5428..9fffa7194 100644 --- a/thirdparty/swiftmailer/Swift/Transport/SimpleMailInvoker.php +++ b/thirdparty/swiftmailer/Swift/Transport/SimpleMailInvoker.php @@ -5,6 +5,8 @@ * It has been slightly modified to meet phpcs standards */ +use SilverStripe\Dev\Deprecation; + /* * This file is part of SwiftMailer. * (c) 2004-2009 Chris Corbyn @@ -17,12 +19,20 @@ * This is the implementation class for {@link Swift_Transport_MailInvoker}. * * @author Chris Corbyn + * @deprecated 4.12.0 Sending email using the mail() function is deprecated. Use sendmail or SMTP instead */ // @codingStandardsIgnoreStart // ignore missing namespace class Swift_Transport_SimpleMailInvoker implements Swift_Transport_MailInvoker // @codingStandardsIgnoreEnd* It has been slightly modified to meet phpcs standards { + public function __construct() + { + Deprecation::withNoReplacement(function () { + Deprecation::notice('4.12.0', 'Sending email using the mail() function is deprecated. Use sendmail or SMTP instead', Deprecation::SCOPE_CLASS); + }); + } + /** * Send mail via the mail() function. * From 05674adf512006b9430f3b68efee960c8c1b529c Mon Sep 17 00:00:00 2001 From: Sabina Talipova Date: Tue, 28 Feb 2023 21:31:46 +1300 Subject: [PATCH 16/20] ENH Updated deprecation warning message --- src/ORM/Connect/MySQLDatabase.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/ORM/Connect/MySQLDatabase.php b/src/ORM/Connect/MySQLDatabase.php index 9200f3a8c..6da201f0d 100644 --- a/src/ORM/Connect/MySQLDatabase.php +++ b/src/ORM/Connect/MySQLDatabase.php @@ -367,14 +367,16 @@ class MySQLDatabase extends Database implements TransactionManager public function transactionEnd($chain = false) { - $result = $this->getTransactionManager()->transactionEnd(); - if ($chain) { - Deprecation::notice('4.4', '$chain argument is deprecated'); + Deprecation::notice( + '4.4.0', + '$chain argument in ' . __METHOD__ . ' is deprecated', + Deprecation::SCOPE_GLOBAL + ); return $this->getTransactionManager()->transactionStart(); } - return $result; + return $this->getTransactionManager()->transactionEnd(); } /** From e3a94b9d10790c66b0ac247f3dc2f258b57c0aaa Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Wed, 1 Mar 2023 15:19:07 +1300 Subject: [PATCH 17/20] FIX Ensure getters and setters are respected (#10708) --- src/ORM/FieldType/DBBoolean.php | 6 +- src/ORM/FieldType/DBComposite.php | 8 +- src/ORM/FieldType/DBDecimal.php | 8 +- src/ORM/FieldType/DBField.php | 6 +- src/ORM/FieldType/DBPercentage.php | 2 +- tests/php/ORM/DBFieldTest.php | 73 +++++++++++++++++++ tests/php/ORM/DBFieldTest/TestDataObject.php | 29 ++++++++ tests/php/ORM/DBFieldTest/TestDbField.php | 42 +++++++++++ tests/php/ORM/DataObjectTest.php | 11 +++ .../ORM/DataObjectTest/SettersAndGetters.php | 25 +++++++ 10 files changed, 203 insertions(+), 7 deletions(-) create mode 100644 tests/php/ORM/DBFieldTest/TestDataObject.php create mode 100644 tests/php/ORM/DBFieldTest/TestDbField.php create mode 100644 tests/php/ORM/DataObjectTest/SettersAndGetters.php diff --git a/src/ORM/FieldType/DBBoolean.php b/src/ORM/FieldType/DBBoolean.php index a0b485f5d..46374682c 100644 --- a/src/ORM/FieldType/DBBoolean.php +++ b/src/ORM/FieldType/DBBoolean.php @@ -46,7 +46,11 @@ class DBBoolean extends DBField { $fieldName = $this->name; if ($fieldName) { - $dataObject->setField($fieldName, $this->value ? 1 : 0); + if ($this->value instanceof DBField) { + $this->value->saveInto($dataObject); + } else { + $dataObject->__set($fieldName, $this->value ? 1 : 0); + } } else { $class = static::class; throw new \RuntimeException("DBField::saveInto() Called on a nameless '$class' object"); diff --git a/src/ORM/FieldType/DBComposite.php b/src/ORM/FieldType/DBComposite.php index a4d936b2e..2447f3df9 100644 --- a/src/ORM/FieldType/DBComposite.php +++ b/src/ORM/FieldType/DBComposite.php @@ -221,8 +221,12 @@ abstract class DBComposite extends DBField { foreach ($this->compositeDatabaseFields() as $field => $spec) { // Save into record - $key = $this->getName() . $field; - $dataObject->setField($key, $this->getField($field)); + if ($this->value instanceof DBField) { + $this->value->saveInto($dataObject); + } else { + $key = $this->getName() . $field; + $dataObject->__set($key, $this->getField($field)); + } } } diff --git a/src/ORM/FieldType/DBDecimal.php b/src/ORM/FieldType/DBDecimal.php index a8c9df99b..dcf2d7e80 100644 --- a/src/ORM/FieldType/DBDecimal.php +++ b/src/ORM/FieldType/DBDecimal.php @@ -88,8 +88,12 @@ class DBDecimal extends DBField $fieldName = $this->name; if ($fieldName) { - $value = (float) preg_replace('/[^0-9.\-\+]/', '', $this->value ?? ''); - $dataObject->setField($fieldName, $value); + if ($this->value instanceof DBField) { + $this->value->saveInto($dataObject); + } else { + $value = (float) preg_replace('/[^0-9.\-\+]/', '', $this->value ?? ''); + $dataObject->__set($fieldName, $value); + } } else { throw new \UnexpectedValueException( "DBField::saveInto() Called on a nameless '" . static::class . "' object" diff --git a/src/ORM/FieldType/DBField.php b/src/ORM/FieldType/DBField.php index 0c96442fc..520ce73d8 100644 --- a/src/ORM/FieldType/DBField.php +++ b/src/ORM/FieldType/DBField.php @@ -542,7 +542,11 @@ abstract class DBField extends ViewableData implements DBIndexable "DBField::saveInto() Called on a nameless '" . static::class . "' object" ); } - $dataObject->setField($fieldName, $this->value); + if ($this->value instanceof self) { + $this->value->saveInto($dataObject); + } else { + $dataObject->__set($fieldName, $this->value); + } } /** diff --git a/src/ORM/FieldType/DBPercentage.php b/src/ORM/FieldType/DBPercentage.php index dca55431c..1abf613a2 100644 --- a/src/ORM/FieldType/DBPercentage.php +++ b/src/ORM/FieldType/DBPercentage.php @@ -45,7 +45,7 @@ class DBPercentage extends DBDecimal $fieldName = $this->name; if ($fieldName && $dataObject->$fieldName > 1.0) { - $dataObject->setField($fieldName, 1.0); + $dataObject->__set($fieldName, 1.0); } } } diff --git a/tests/php/ORM/DBFieldTest.php b/tests/php/ORM/DBFieldTest.php index 4c106b7d9..8168137b5 100644 --- a/tests/php/ORM/DBFieldTest.php +++ b/tests/php/ORM/DBFieldTest.php @@ -27,6 +27,7 @@ use SilverStripe\ORM\FieldType\DBTime; use SilverStripe\ORM\FieldType\DBVarchar; use SilverStripe\ORM\FieldType\DBText; use SilverStripe\Dev\SapphireTest; +use SilverStripe\ORM\FieldType\DBField; use SilverStripe\ORM\FieldType\DBYear; /** @@ -34,6 +35,9 @@ use SilverStripe\ORM\FieldType\DBYear; */ class DBFieldTest extends SapphireTest { + protected static $extra_dataobjects = [ + DBFieldTest\TestDataObject::class, + ]; /** * Test the nullValue() method on DBField. @@ -322,4 +326,73 @@ class DBFieldTest extends SapphireTest $this->assertEquals('

ÅÄÖ

', DBHTMLText::create_field('HTMLFragment', '

åäö

')->UpperCase()); $this->assertEquals('

åäö

', DBHTMLText::create_field('HTMLFragment', '

ÅÄÖ

')->LowerCase()); } + + public function testSaveInto() + { + $obj = new DBFieldTest\TestDataObject(); + /** @var DBField $field */ + $field = $obj->dbObject('Title'); + $field->setValue('New Value'); + $field->saveInto($obj); + + $this->assertEquals('New Value', $obj->getField('Title')); + $this->assertEquals(1, $field->saveIntoCalledCount); + $this->assertEquals(1, $obj->setFieldCalledCount); + } + + public function testSaveIntoNoRecursion() + { + $obj = new DBFieldTest\TestDataObject(); + /** @var DBField $field */ + $field = $obj->dbObject('Title'); + $value = new DBFieldTest\TestDbField('Title'); + $value->setValue('New Value'); + $field->setValue($value); + $field->saveInto($obj); + + $this->assertEquals('New Value', $obj->getField('Title')); + $this->assertEquals(1, $field->saveIntoCalledCount); + $this->assertEquals(1, $obj->setFieldCalledCount); + } + + public function testSaveIntoAsProperty() + { + $obj = new DBFieldTest\TestDataObject(); + /** @var DBField $field */ + $field = $obj->dbObject('Title'); + $field->setValue('New Value'); + $obj->Title = $field; + + $this->assertEquals('New Value', $obj->getField('Title')); + $this->assertEquals(1, $field->saveIntoCalledCount); + // Called twice because $obj->setField($field) => $field->saveInto() => $obj->setField('New Value') + $this->assertEquals(2, $obj->setFieldCalledCount); + } + + public function testSaveIntoNoRecursionAsProperty() + { + $obj = new DBFieldTest\TestDataObject(); + /** @var DBField $field */ + $field = $obj->dbObject('Title'); + $value = new DBFieldTest\TestDbField('Title'); + $value->setValue('New Value'); + $field->setValue($value); + $obj->Title = $field; + + $this->assertEquals('New Value', $obj->getField('Title')); + $this->assertEquals(1, $field->saveIntoCalledCount); + // Called twice because $obj->setField($field) => $field->saveInto() => $obj->setField('New Value') + $this->assertEquals(2, $obj->setFieldCalledCount); + } + + public function testSaveIntoRespectsSetters() + { + $obj = new DBFieldTest\TestDataObject(); + /** @var DBField $field */ + $field = $obj->dbObject('MyTestField'); + $field->setValue('New Value'); + $obj->MyTestField = $field; + + $this->assertEquals('new value', $obj->getField('MyTestField')); + } } diff --git a/tests/php/ORM/DBFieldTest/TestDataObject.php b/tests/php/ORM/DBFieldTest/TestDataObject.php new file mode 100644 index 000000000..4d2efd374 --- /dev/null +++ b/tests/php/ORM/DBFieldTest/TestDataObject.php @@ -0,0 +1,29 @@ + TestDbField::class, + 'MyTestField' => TestDbField::class, + ]; + + public $setFieldCalledCount = 0; + + public function setField($fieldName, $val) + { + $this->setFieldCalledCount++; + return parent::setField($fieldName, $val); + } + + public function setMyTestField($val) + { + return $this->setField('MyTestField', strtolower($val)); + } +} diff --git a/tests/php/ORM/DBFieldTest/TestDbField.php b/tests/php/ORM/DBFieldTest/TestDbField.php new file mode 100644 index 000000000..deb9773c4 --- /dev/null +++ b/tests/php/ORM/DBFieldTest/TestDbField.php @@ -0,0 +1,42 @@ +get(MySQLDatabase::class, 'charset'); + $collation = Config::inst()->get(MySQLDatabase::class, 'collation'); + + $parts = [ + 'datatype' => 'varchar', + 'precision' => 255, + 'character set' => $charset, + 'collate' => $collation, + 'arrayValue' => $this->arrayValue + ]; + + $values = [ + 'type' => 'varchar', + 'parts' => $parts + ]; + + DB::require_field($this->tableName, $this->name, $values); + } + + public $saveIntoCalledCount = 0; + + public function saveInto($dataObject) + { + $this->saveIntoCalledCount++; + return parent::saveInto($dataObject); + } +} diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php index 493840301..4139291ca 100644 --- a/tests/php/ORM/DataObjectTest.php +++ b/tests/php/ORM/DataObjectTest.php @@ -66,6 +66,7 @@ class DataObjectTest extends SapphireTest DataObjectTest\TreeNode::class, DataObjectTest\OverriddenDataObject::class, DataObjectTest\InjectedDataObject::class, + DataObjectTest\SettersAndGetters::class, ]; protected function setUp(): void @@ -2667,4 +2668,14 @@ class DataObjectTest extends SapphireTest $vals = ['25.25', '50.00', '75.00', '100.50']; $this->assertSame(array_combine($vals ?? [], $vals ?? []), $obj->dbObject('MyEnumWithDots')->enumValues()); } + + public function testSettersAndGettersAreRespected() + { + $obj = new DataObjectTest\SettersAndGetters(); + $obj->MyTestField = 'Some Value'; + // Setter overrides it with all lower case + $this->assertSame('some value', $obj->getField('MyTestField')); + // Getter overrides it with all upper case + $this->assertSame('SOME VALUE', $obj->MyTestField); + } } diff --git a/tests/php/ORM/DataObjectTest/SettersAndGetters.php b/tests/php/ORM/DataObjectTest/SettersAndGetters.php new file mode 100644 index 000000000..7d6ff9059 --- /dev/null +++ b/tests/php/ORM/DataObjectTest/SettersAndGetters.php @@ -0,0 +1,25 @@ + 'Varchar(255)', + ]; + + public function setMyTestField($val) + { + $this->setField('MyTestField', strtolower($val)); + } + + public function getMyTestField() + { + return strtoupper($this->getField('MyTestField')); + } +} From 6585d499f5aca7f03e87710aeb8a277cb7dc7e5e Mon Sep 17 00:00:00 2001 From: Florian Thoma Date: Mon, 13 Feb 2023 16:18:44 +1100 Subject: [PATCH 18/20] FIX Convert slashes in paths when getting list of classes for file/folder This is to support the mechanism working on all operating systems where Windows may produce a mix of forward and backward slashes in some paths. For working with the files it may not be a problem, but for exact string comparison the path delimiters need to be unified. --- src/Core/ClassInfo.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Core/ClassInfo.php b/src/Core/ClassInfo.php index 38542c1d3..6ac4c58d4 100644 --- a/src/Core/ClassInfo.php +++ b/src/Core/ClassInfo.php @@ -301,14 +301,14 @@ class ClassInfo */ public static function classes_for_file($filePath) { - $absFilePath = Director::getAbsFile($filePath); + $absFilePath = Convert::slashes(Director::getAbsFile($filePath)); $classManifest = ClassLoader::inst()->getManifest(); $classes = $classManifest->getClasses(); $classNames = $classManifest->getClassNames(); $matchedClasses = []; foreach ($classes as $lowerClass => $compareFilePath) { - if (strcasecmp($absFilePath ?? '', $compareFilePath ?? '') === 0) { + if (strcasecmp($absFilePath, Convert::slashes($compareFilePath ?? '')) === 0) { $matchedClasses[$lowerClass] = $classNames[$lowerClass]; } } @@ -324,14 +324,14 @@ class ClassInfo */ public static function classes_for_folder($folderPath) { - $absFolderPath = Director::getAbsFile($folderPath); + $absFolderPath = Convert::slashes(Director::getAbsFile($folderPath)); $classManifest = ClassLoader::inst()->getManifest(); $classes = $classManifest->getClasses(); $classNames = $classManifest->getClassNames(); $matchedClasses = []; foreach ($classes as $lowerClass => $compareFilePath) { - if (stripos($compareFilePath ?? '', $absFolderPath ?? '') === 0) { + if (stripos(Convert::slashes($compareFilePath ?? ''), $absFolderPath) === 0) { $matchedClasses[$lowerClass] = $classNames[$lowerClass]; } } From 403f924d22455d2ba8f927888f7ee6d88b55e034 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Thu, 2 Mar 2023 09:56:40 +1300 Subject: [PATCH 19/20] BUG Update RelatedDataService to properly escape ClassName in Polymorphic relations (#10713) --- src/ORM/RelatedData/StandardRelatedDataService.php | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/ORM/RelatedData/StandardRelatedDataService.php b/src/ORM/RelatedData/StandardRelatedDataService.php index 04501fc23..c6be4632d 100644 --- a/src/ORM/RelatedData/StandardRelatedDataService.php +++ b/src/ORM/RelatedData/StandardRelatedDataService.php @@ -394,14 +394,7 @@ class StandardRelatedDataService implements RelatedDataService */ private function prepareClassNameLiteral(string $value): string { - $c = chr(92); - $escaped = str_replace($c ?? '', "{$c}{$c}", $value ?? ''); - // postgres - if (stripos(get_class(DB::get_conn()), 'postgres') !== false) { - return "E'{$escaped}'"; - } - // mysql - return "'{$escaped}'"; + return DB::get_conn()->quoteString($value); } /** From 66561ccb49e196bd652b1fc2112c5aeda062354c Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Thu, 2 Mar 2023 11:05:35 +1300 Subject: [PATCH 20/20] FIX Correctly deprecation Sources.module_priority (#10711) This config was deprecated back in #7154 and hasn't been used since --- src/i18n/Data/Sources.php | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/i18n/Data/Sources.php b/src/i18n/Data/Sources.php index e7a233ab9..e02e73999 100644 --- a/src/i18n/Data/Sources.php +++ b/src/i18n/Data/Sources.php @@ -26,6 +26,7 @@ class Sources implements Resettable * * @config * @var array + * @deprecated 4.0.0 Use SilverStripe\Core\Manifest\ModuleManifest.module_priority instead */ private static $module_priority = []; @@ -36,15 +37,7 @@ class Sources implements Resettable */ public function getSortedModules() { - $i18nOrder = Sources::config()->uninherited('module_priority'); $sortedModules = []; - if ($i18nOrder) { - Deprecation::notice('5.0', sprintf( - '%s.module_priority is deprecated. Use %s.module_priority instead.', - __CLASS__, - ModuleManifest::class - )); - } foreach (ModuleLoader::inst()->getManifest()->getModules() as $module) { $sortedModules[$module->getName()] = $module->getPath();