[TASK] Simplify PackageManager caching 82/58282/4
authorBenjamin Franzke <bfr@qbus.de>
Sat, 15 Sep 2018 07:31:32 +0000 (09:31 +0200)
committerSusanne Moog <susanne.moog@typo3.org>
Tue, 18 Sep 2018 06:44:17 +0000 (08:44 +0200)
We do not want to pollute $GLOBALS to be able to inject PackageManager
as $GLOBALS['TYPO3_currentPackageManager']. to cache-restored Package
objects (during unserialize).
Therefore this patch drops the PackageManager dependency from the Package
class.

The global injected PackageManager was used to calculate the
package metadata on demand. Package metadata is a small array
that doesn't harm to be created during Package object construction,
(it's cached afterwards anyway).
This allows us to drop the PackageManager reference, which
simplifies serialize/unserialize logic (we can drop __sleep and __wakeup).

We also combine the PackageManger and PackageObjects cache
into one cache file instead of two. There is no advantage
in having to read two files from the filesystem. The prior argument
(introduced with the initial PackageManager patch in 2013) "so PHP does
not have to parse the serialized string" is actually invalid:
The serialized string has to be parsed anyway.
PHP did not need to parse the var_export'd variant, true.
BUT: The var_export'd variant is theoretically php opcache'able, while
reading a file and passing the file contents to serialize() is not.
That means the previous solution actually hampered native optimizations.

Ideally we could drop the serialize/unserialize thing and just use
var_export for Package objects, but for that to work the Package class
(and all of it's properties) would need to implement the magic
__set_state() method (which is used by var_export to create objects).
That's currently not possible, because the composerManifest (which
is read from json_decode) is a stdClass and does not provide a
__set_state() method. We'd need to rewrite all that code to an array
or so. Maybe something for a later patch.

As a drive-by fix we now hash the TYPO3_version value into the
cacheEntryIdentifier like other core caches do.

Change-Id: I764dc92c64165ede24c8020c44cd2360b3faa00c
Releases: master
Resolves: #86261
Reviewed-on: https://review.typo3.org/58282
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Benni Mack <benni@typo3.org>
Tested-by: Benni Mack <benni@typo3.org>
Reviewed-by: Susanne Moog <susanne.moog@typo3.org>
Tested-by: Susanne Moog <susanne.moog@typo3.org>
typo3/sysext/core/Classes/Package/Package.php
typo3/sysext/core/Classes/Package/PackageManager.php

index e604f62..d7db187 100644 (file)
@@ -70,11 +70,6 @@ class Package implements PackageInterface
     protected $packageMetaData;
 
     /**
-     * @var PackageManager
-     */
-    protected $packageManager;
-
-    /**
      * Constructor
      *
      * @param PackageManager $packageManager the package manager which knows this package
@@ -95,11 +90,11 @@ class Package implements PackageInterface
         if (substr($packagePath, -1, 1) !== '/') {
             throw new Exception\InvalidPackagePathException(sprintf('The package path "%s" provided for package "%s" has no trailing forward slash.', $packagePath, $packageKey), 1166633722);
         }
-        $this->packageManager = $packageManager;
         $this->packageKey = $packageKey;
         $this->packagePath = $packagePath;
         $this->composerManifest = $packageManager->getComposerManifest($this->packagePath);
         $this->loadFlagsFromComposerManifest();
+        $this->createPackageMetadata($packageManager);
     }
 
     /**
@@ -119,6 +114,40 @@ class Package implements PackageInterface
     }
 
     /**
+     * Creates the package meta data object of this package.
+     *
+     * @param PackageManager $packageManager
+     */
+    protected function createPackageMetaData(PackageManager $packageManager)
+    {
+        $this->packageMetaData = new MetaData($this->getPackageKey());
+        $this->packageMetaData->setDescription($this->getValueFromComposerManifest('description'));
+        $this->packageMetaData->setVersion($this->getValueFromComposerManifest('version'));
+        $requirements = $this->getValueFromComposerManifest('require');
+        if ($requirements !== null) {
+            foreach ($requirements as $requirement => $version) {
+                $packageKey = $packageManager->getPackageKeyFromComposerName($requirement);
+                // dynamically migrate 'cms' dependency to 'core' dependency
+                // see also \TYPO3\CMS\Extensionmanager\Utility\ExtensionModelUtility::convertDependenciesToObjects
+                if ($packageKey === 'cms') {
+                    trigger_error('Extension "' . $this->packageKey . '" defines a dependency on ext:cms, which has been removed. Please remove the dependency.', E_USER_DEPRECATED);
+                    $packageKey = 'core';
+                }
+                $constraint = new MetaData\PackageConstraint(MetaData::CONSTRAINT_TYPE_DEPENDS, $packageKey);
+                $this->packageMetaData->addConstraint($constraint);
+            }
+        }
+        $suggestions = $this->getValueFromComposerManifest('suggest');
+        if ($suggestions !== null) {
+            foreach ($suggestions as $suggestion => $version) {
+                $packageKey = $packageManager->getPackageKeyFromComposerName($suggestion);
+                $constraint = new MetaData\PackageConstraint(MetaData::CONSTRAINT_TYPE_SUGGESTS, $packageKey);
+                $this->packageMetaData->addConstraint($constraint);
+            }
+        }
+    }
+
+    /**
      * @return bool
      */
     public function isPartOfFactoryDefault()
@@ -185,33 +214,6 @@ class Package implements PackageInterface
      */
     public function getPackageMetaData()
     {
-        if ($this->packageMetaData === null) {
-            $this->packageMetaData = new MetaData($this->getPackageKey());
-            $this->packageMetaData->setDescription($this->getValueFromComposerManifest('description'));
-            $this->packageMetaData->setVersion($this->getValueFromComposerManifest('version'));
-            $requirements = $this->getValueFromComposerManifest('require');
-            if ($requirements !== null) {
-                foreach ($requirements as $requirement => $version) {
-                    $packageKey = $this->packageManager->getPackageKeyFromComposerName($requirement);
-                    // dynamically migrate 'cms' dependency to 'core' dependency
-                    // see also \TYPO3\CMS\Extensionmanager\Utility\ExtensionModelUtility::convertDependenciesToObjects
-                    if ($packageKey === 'cms') {
-                        trigger_error('Extension "' . $this->packageKey . '" defines a dependency on ext:cms, which has been removed. Please remove the dependency.', E_USER_DEPRECATED);
-                        $packageKey = 'core';
-                    }
-                    $constraint = new MetaData\PackageConstraint(MetaData::CONSTRAINT_TYPE_DEPENDS, $packageKey);
-                    $this->packageMetaData->addConstraint($constraint);
-                }
-            }
-            $suggestions = $this->getValueFromComposerManifest('suggest');
-            if ($suggestions !== null) {
-                foreach ($suggestions as $suggestion => $version) {
-                    $packageKey = $this->packageManager->getPackageKeyFromComposerName($suggestion);
-                    $constraint = new MetaData\PackageConstraint(MetaData::CONSTRAINT_TYPE_SUGGESTS, $packageKey);
-                    $this->packageMetaData->addConstraint($constraint);
-                }
-            }
-        }
         return $this->packageMetaData;
     }
 
@@ -246,40 +248,4 @@ class Package implements PackageInterface
         }
         return $value;
     }
-
-    /**
-     * Added by TYPO3 CMS
-     *
-     * The package caching serializes package objects.
-     * The package manager instance may not be serialized
-     * as a fresh instance is created upon every request.
-     *
-     * This method will be removed once the package is
-     * released of the package manager dependency.
-     *
-     * @return array
-     */
-    public function __sleep()
-    {
-        $properties = get_class_vars(static::class);
-        unset($properties['packageManager']);
-        return array_keys($properties);
-    }
-
-    /**
-     * Added by TYPO3 CMS
-     *
-     * The package caching deserializes package objects.
-     * A fresh package manager instance has to be set
-     * during bootstrapping.
-     *
-     * This method will be removed once the package is
-     * released of the package manager dependency.
-     */
-    public function __wakeup()
-    {
-        if (isset($GLOBALS['TYPO3_currentPackageManager'])) {
-            $this->packageManager = $GLOBALS['TYPO3_currentPackageManager'];
-        }
-    }
 }
index a82b4da..8e9a0f1 100644 (file)
@@ -26,7 +26,6 @@ use TYPO3\CMS\Core\SingletonInterface;
 use TYPO3\CMS\Core\Utility\ArrayUtility;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Core\Utility\PathUtility;
-use TYPO3\CMS\Core\Utility\StringUtility;
 
 /**
  * The default TYPO3 Package Manager
@@ -157,7 +156,7 @@ class PackageManager implements SingletonInterface
         if ($this->cacheIdentifier === null) {
             $mTime = @filemtime($this->packageStatesPathAndFilename);
             if ($mTime !== false) {
-                $this->cacheIdentifier = md5($this->packageStatesPathAndFilename . $mTime);
+                $this->cacheIdentifier = md5(TYPO3_version . $this->packageStatesPathAndFilename . $mTime);
             } else {
                 $this->cacheIdentifier = null;
             }
@@ -181,17 +180,14 @@ class PackageManager implements SingletonInterface
     {
         $cacheEntryIdentifier = $this->getCacheEntryIdentifier();
         if ($cacheEntryIdentifier !== null && !$this->coreCache->has($cacheEntryIdentifier)) {
-            // Package objects get their own cache entry, so PHP does not have to parse the serialized string
-            $packageObjectsCacheEntryIdentifier = StringUtility::getUniqueId('PackageObjects_');
             // Build cache file
             $packageCache = [
                 'packageStatesConfiguration' => $this->packageStatesConfiguration,
                 'packageAliasMap' => $this->packageAliasMap,
                 'loadedExtArray' => $GLOBALS['TYPO3_LOADED_EXT'],
                 'composerNameToPackageKeyMap' => $this->composerNameToPackageKeyMap,
-                'packageObjectsCacheEntryIdentifier' => $packageObjectsCacheEntryIdentifier
+                'packageObjects' => serialize($this->packages),
             ];
-            $this->coreCache->set($packageObjectsCacheEntryIdentifier, serialize($this->packages));
             $this->coreCache->set(
                 $cacheEntryIdentifier,
                 'return ' . PHP_EOL . var_export($packageCache, true) . ';'
@@ -216,12 +212,15 @@ class PackageManager implements SingletonInterface
         }
         $this->packageAliasMap = $packageCache['packageAliasMap'];
         $this->composerNameToPackageKeyMap = $packageCache['composerNameToPackageKeyMap'];
+        $this->packages = unserialize($packageCache['packageObjects'], [
+            'allowed_classes' => [
+                Package::class,
+                MetaData::class,
+                MetaData\PackageConstraint::class,
+                \stdClass::class,
+            ]
+        ]);
         $GLOBALS['TYPO3_LOADED_EXT'] = $packageCache['loadedExtArray'];
-        $GLOBALS['TYPO3_currentPackageManager'] = $this;
-        // Strip off PHP Tags from Php Cache Frontend
-        $packageObjects = substr(substr($this->coreCache->get($packageCache['packageObjectsCacheEntryIdentifier']), 6), 0, -2);
-        $this->packages = unserialize($packageObjects);
-        unset($GLOBALS['TYPO3_currentPackageManager']);
     }
 
     /**