From b97f50bcc664856e1e7ff1ba85cb790d075bfdd0 Mon Sep 17 00:00:00 2001
From: Christian Kuhn <lolli@schwarzbu.ch>
Date: Wed, 30 Aug 2023 02:39:34 +0200
Subject: [PATCH] [TASK] Remove DataHandler->checkStoredRecord()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Well, that functionality is hilarious: Sometimes,
when the DataHandler updates or inserts database
rows, it calls checkStoredRecord() which fetches
the just written row to verify the row has
actually been written correctly.
* The implementation is inconsistent, DH does
not do this for all update queries.
* The default configuration is to fetch the row,
but to *not* check the fields, rendering the
entire thing useless, but still have the
additional query.
* *If* DH would actually find some collision, it
will bury that information as entry in sys_log.
* We fiddled with this in #79438 for v8 already to
allow suppressing overhead via TYPO3_CONF_VARS.
These toggles are widely unknown and most likely
used very seldom.
* Inserting invalid data a DB can not persist most
likely raises doctrine exceptions, especially with
many database instances being configured in "strict"
mode nowadays.
* With our current efforts to configure database
columns automatically by auto-creating them from
TCA, we further reduce the risk of columns not
being configured correctly for given data.
The patch:
* Removes (@internal) DataHandler->checkStoredRecord()
* Keeps two related DataHandler properties as b/w
compat layer, but renders them unused.
* Removes two related TYPO3_CONF_VARS.
Resolves: #101793
Related: #79438
Related: #101553
Releases: main
Change-Id: Ie93eddda48057b844067b654e327b8a371b197fc
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/80752
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Tested-by: Stefan B�rk <stefan@buerk.tech>
Tested-by: Oliver Klee <typo3-coding@oliverklee.de>
Tested-by: core-ci <typo3@b13.com>
Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Stefan B�rk <stefan@buerk.tech>
---
.../core/Classes/DataHandling/DataHandler.php | 112 +-----------------
.../Configuration/DefaultConfiguration.php | 2 -
.../DefaultConfigurationDescription.yaml | 6 -
...ataHandlerCheckStoredRecordsProperties.rst | 63 ++++++++++
.../SilentConfigurationUpgradeService.php | 3 +
.../Php/PropertyPublicMatcher.php | 10 ++
6 files changed, 82 insertions(+), 114 deletions(-)
create mode 100644 typo3/sysext/core/Documentation/Changelog/13.0/Deprecation-101793-DataHandlerCheckStoredRecordsProperties.rst
diff --git a/typo3/sysext/core/Classes/DataHandling/DataHandler.php b/typo3/sysext/core/Classes/DataHandling/DataHandler.php
index d6df6c197a6a..ce7067b0d73d 100644
--- a/typo3/sysext/core/Classes/DataHandling/DataHandler.php
+++ b/typo3/sysext/core/Classes/DataHandling/DataHandler.php
@@ -127,19 +127,9 @@ class DataHandler implements LoggerAwareInterface
*/
public $reverseOrder = false;
- /**
- * This will read the record after having updated or inserted it. If anything is not properly submitted an error
- * is written to the log. This feature consumes extra time by selecting records
- *
- * @var bool
- */
+ /** @deprecated Unused. Will be removed with TYPO3 v14. */
public $checkStoredRecords = true;
-
- /**
- * If set, values '' and 0 will equal each other when the stored records are checked.
- *
- * @var bool
- */
+ /** @deprecated Unused. Will be removed with TYPO3 v14. */
public $checkStoredRecords_loose = true;
/**
@@ -602,8 +592,6 @@ class DataHandler implements LoggerAwareInterface
*/
public function __construct(ReferenceIndexUpdater $referenceIndexUpdater = null)
{
- $this->checkStoredRecords = (bool)$GLOBALS['TYPO3_CONF_VARS']['BE']['checkStoredRecords'];
- $this->checkStoredRecords_loose = (bool)$GLOBALS['TYPO3_CONF_VARS']['BE']['checkStoredRecordsLoose'];
$this->runtimeCache = $this->getRuntimeCache();
$this->pagePermissionAssembler = GeneralUtility::makeInstance(PagePermissionAssembler::class, $GLOBALS['TYPO3_CONF_VARS']['BE']['defaultPermissions']);
if ($referenceIndexUpdater === null) {
@@ -7737,12 +7725,8 @@ class DataHandler implements LoggerAwareInterface
$historyEntryId = $this->getRecordHistoryStore()->modifyRecord($table, $id, $this->historyRecords[$table . ':' . $id], $this->correlationId);
}
if ($this->enableLogging) {
- if ($this->checkStoredRecords) {
- $newRow = $this->checkStoredRecord($table, $id, $fieldArray, SystemLogDatabaseAction::UPDATE) ?? [];
- } else {
- $newRow = $fieldArray;
- $newRow['uid'] = $id;
- }
+ $newRow = $fieldArray;
+ $newRow['uid'] = $id;
// Set log entry:
$propArr = $this->getRecordPropertiesFromRow($table, $newRow);
$isOfflineVersion = (bool)($newRow['t3ver_oid'] ?? 0);
@@ -7818,13 +7802,8 @@ class DataHandler implements LoggerAwareInterface
}
$newRow = [];
if ($this->enableLogging) {
- // Checking the record is properly saved if configured
- if ($this->checkStoredRecords) {
- $newRow = $this->checkStoredRecord($table, $id, $fieldArray, SystemLogDatabaseAction::INSERT) ?? [];
- } else {
- $newRow = $fieldArray;
- $newRow['uid'] = $id;
- }
+ $newRow = $fieldArray;
+ $newRow['uid'] = $id;
}
// Update reference index:
$this->updateRefIndex($table, $id);
@@ -7854,85 +7833,6 @@ class DataHandler implements LoggerAwareInterface
return null;
}
- /**
- * Checking stored record to see if the written values are properly updated.
- *
- * @param string $table Record table name
- * @param int $id Record uid
- * @param array $fieldArray Array of field=>value pairs to insert/update
- * @param int $action Action, for logging only.
- * @return array|null Selected row
- * @see insertDB()
- * @see updateDB()
- * @internal should only be used from within DataHandler
- */
- public function checkStoredRecord($table, $id, $fieldArray, $action)
- {
- $id = (int)$id;
- if (is_array($GLOBALS['TCA'][$table]) && $id) {
- $tcaTableColumns = $GLOBALS['TCA'][$table]['columns'] ?? [];
- $queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)->getQueryBuilderForTable($table);
- $queryBuilder->getRestrictions()->removeAll();
-
- $row = $queryBuilder
- ->select('*')
- ->from($table)
- ->where($queryBuilder->expr()->eq('uid', $queryBuilder->createNamedParameter($id, Connection::PARAM_INT)))
- ->executeQuery()
- ->fetchAssociative();
-
- if (!empty($row)) {
- $row = BackendUtility::convertDatabaseRowValuesToPhp($table, $row);
- // Traverse array of values that was inserted into the database and compare with the actually stored value:
- $errors = [];
- foreach ($fieldArray as $key => $value) {
- if (!$this->checkStoredRecords_loose || $value || $row[$key]) {
- // @todo Check explicitly for one type is fishy. However needed to avoid array to string
- // conversion errors. Find a better way do handle this.
- if (($tcaTableColumns[$key]['config']['type'] ?? '') === 'json') {
- // To ensure a proper comparison we need to sort the array structure based on array keys
- // in a recursive manner. Otherwise, we would emit an error just because the ordering was
- // different. This must be done for value and the value in the row to be safe.
- if (is_array($value)) {
- ArrayUtility::naturalKeySortRecursive($value);
- }
- if (is_array($row[$key])) {
- ArrayUtility::naturalKeySortRecursive($row[$key]);
- }
- if ($row[$key] !== $value) {
- $errors[] = $key;
- }
- } elseif (is_float($row[$key])) {
- // if the database returns the value as double, compare it as double
- if ((float)$value !== (float)$row[$key]) {
- $errors[] = $key;
- }
- } else {
- if ((string)$value !== (string)$row[$key]) {
- // The is_numeric check catches cases where we want to store a float/double value
- // and database returns the field as a string with the least required amount of
- // significant digits, i.e. "0.00" being saved and "0" being read back.
- if (is_numeric($value) && is_numeric($row[$key])) {
- if ((float)$value === (float)$row[$key]) {
- continue;
- }
- }
- $errors[] = $key;
- }
- }
- }
- }
- // Set log message if there were fields with unmatching values:
- if (!empty($errors)) {
- $this->log($table, $id, $action, 0, SystemLogErrorClassification::USER_ERROR, 'These fields of record {id} in table "{table}" have not been saved correctly: {fields}. The values might have changed due to type casting of the database', -1, ['id' => $id, 'table' => $table, 'fields' => implode(', ', $errors)]);
- }
- // Return selected rows:
- return $row;
- }
- }
- return null;
- }
-
/**
* Setting sys_history record, based on content previously set in $this->historyRecords[$table . ':' . $id] (by compareFieldArrayWithCurrentAndUnset())
*
diff --git a/typo3/sysext/core/Configuration/DefaultConfiguration.php b/typo3/sysext/core/Configuration/DefaultConfiguration.php
index 735205e34854..152d4fe0970c 100644
--- a/typo3/sysext/core/Configuration/DefaultConfiguration.php
+++ b/typo3/sysext/core/Configuration/DefaultConfiguration.php
@@ -1321,8 +1321,6 @@ return [
'disable_exec_function' => false,
'compressionLevel' => 0,
'installToolPassword' => '',
- 'checkStoredRecords' => true,
- 'checkStoredRecordsLoose' => true,
'contentSecurityPolicyReportingUrl' => '',
'defaultUserTSconfig' => 'options.enableBookmarks=1
options.file_list.enableDisplayThumbnails=selectable
diff --git a/typo3/sysext/core/Configuration/DefaultConfigurationDescription.yaml b/typo3/sysext/core/Configuration/DefaultConfigurationDescription.yaml
index 4f80c36066db..c62a84ee3d2d 100644
--- a/typo3/sysext/core/Configuration/DefaultConfigurationDescription.yaml
+++ b/typo3/sysext/core/Configuration/DefaultConfigurationDescription.yaml
@@ -360,12 +360,6 @@ BE:
compressionLevel:
type: text
description: 'Determines output compression of BE output. Makes output smaller but slows down the page generation depending on the compression level. Requires a) zlib in your PHP installation and b) special rewrite rules for .css.gz and .js.gz (please see <code>_.htaccess</code> for an example). Range 1-9, where 1 is least compression and 9 is greatest compression. ''true'' as value will set the compression based on the PHP default settings (usually 5). Suggested and most optimal value is 5.'
- checkStoredRecords:
- type: bool
- description: 'If set, values of the record are validated after saving in DataHandler. Disable only if using a database in strict mode.'
- checkStoredRecordsLoose:
- type: bool
- description: 'If set, make a loose comparison ('''' equals 0) when validating record values after saving in DataHandler.'
contentSecurityPolicyReportingUrl:
type: text
description: 'Content-Security-Policy reporting HTTP endpoint, if empty system default will be used'
diff --git a/typo3/sysext/core/Documentation/Changelog/13.0/Deprecation-101793-DataHandlerCheckStoredRecordsProperties.rst b/typo3/sysext/core/Documentation/Changelog/13.0/Deprecation-101793-DataHandlerCheckStoredRecordsProperties.rst
new file mode 100644
index 000000000000..31c2fd592b7a
--- /dev/null
+++ b/typo3/sysext/core/Documentation/Changelog/13.0/Deprecation-101793-DataHandlerCheckStoredRecordsProperties.rst
@@ -0,0 +1,63 @@
+.. include:: /Includes.rst.txt
+
+.. _deprecation-101793-1693356502:
+
+================================================================
+Deprecation: #101793 - DataHandler checkStoredRecords properties
+================================================================
+
+See :issue:`101793`
+
+Description
+===========
+
+The Backend :php:`DataHandler` had a functionality to verify written records
+after they have been persisted in the database and log unexpected collisions.
+
+This feature has been removed since it is rather useless with many databases
+in strict mode nowadays and since the default configuration was to not
+actually check single fields but to still create overhead by always
+querying records from the database without benefit.
+
+Two :php:`TYPO3_CONF_VARS` toggles have been obsoleted:
+
+* :php:`$GLOBALS['TYPO3_CONF_VARS']['BE']['checkStoredRecords']`
+* :php:`$GLOBALS['TYPO3_CONF_VARS']['BE']['checkStoredRecordsLoose']`
+
+Two :php:`DataHandler` properties have been marked as deprecated:
+
+* :php:`TYPO3\CMS\Core\DataHandling\DataHandler->checkStoredRecords`
+* :php:`TYPO3\CMS\Core\DataHandling\DataHandler->checkStoredRecords_loose`
+
+Impact
+======
+
+There should be little to no impact for instances, except some less database
+queries when using the DataHandler. Extensions setting the DataHandler
+properties should stop using them, they will be removed with TYPO3 v14 and
+have no functionality with v13 anymore.
+
+
+Affected installations
+======================
+
+In rare cases, instances with extensions setting the DataHandler properties
+are affected. The extension scanner will find possible usages with a weak
+match.
+
+Instances setting the :php:`TYPO3_CONF_VARS` toggles in :php:`settings.php`
+are updated silently by the install tool during the upgrade process to TYPO3 v13.
+
+
+Migration
+=========
+
+Extensions aiming for compatibility with TYPO3 v12 and v13 can continue to set the
+properties :php:`DataHandler->checkStoredRecords` and :php:`DataHandler->checkStoredRecords_loose`,
+they are kept in v13, but functionality bound to them is removed.
+
+Extensions aiming for compatibility with TYPO3 v13 and above should remove
+usages of :php:`DataHandler->checkStoredRecords` and :php:`DataHandler->checkStoredRecords_loose`,
+they are without functionality in TYPO v13 and will be removed with TYPO3 v14.
+
+.. index:: Database, LocalConfiguration, PHP-API, FullyScanned, ext:core
diff --git a/typo3/sysext/install/Classes/Service/SilentConfigurationUpgradeService.php b/typo3/sysext/install/Classes/Service/SilentConfigurationUpgradeService.php
index 934691420942..d185bebdc077 100644
--- a/typo3/sysext/install/Classes/Service/SilentConfigurationUpgradeService.php
+++ b/typo3/sysext/install/Classes/Service/SilentConfigurationUpgradeService.php
@@ -182,6 +182,9 @@ class SilentConfigurationUpgradeService
// #101037
'BE/languageDebug',
'BE/lang/debug',
+ // #101793
+ 'BE/checkStoredRecords',
+ 'BE/checkStoredRecordsLoose',
];
public function __construct(private readonly ConfigurationManager $configurationManager)
diff --git a/typo3/sysext/install/Configuration/ExtensionScanner/Php/PropertyPublicMatcher.php b/typo3/sysext/install/Configuration/ExtensionScanner/Php/PropertyPublicMatcher.php
index 8bb68975d2a3..841f49516b78 100644
--- a/typo3/sysext/install/Configuration/ExtensionScanner/Php/PropertyPublicMatcher.php
+++ b/typo3/sysext/install/Configuration/ExtensionScanner/Php/PropertyPublicMatcher.php
@@ -1016,4 +1016,14 @@ return [
'Breaking-100963-DeprecatedFunctionalityRemoved.rst',
],
],
+ 'TYPO3\CMS\Core\DataHandling\DataHandler->checkStoredRecords' => [
+ 'restFiles' => [
+ 'Deprecation-101793-DataHandlerCheckStoredRecordsProperties.rst',
+ ],
+ ],
+ 'TYPO3\CMS\Core\DataHandling\DataHandler->checkStoredRecords_loose' => [
+ 'restFiles' => [
+ 'Deprecation-101793-DataHandlerCheckStoredRecordsProperties.rst',
+ ],
+ ],
];
--
GitLab