-
-
Notifications
You must be signed in to change notification settings - Fork 222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TASK: Add and enforce strict_type
declarations
#5240
Conversation
Adds a ``` declare(strict_types=1); ``` declaration to all PHP files of the core that were missing one and implements & defines a new PHPStan rule to enforce them from now on Related: #5239
@@ -246,7 +249,7 @@ public function postProcess(array $evaluateContext, $fusionObject, $output) | |||
} | |||
$cacheTags = $this->buildCacheTags($evaluateContext['configuration'], $evaluateContext['fusionPath'], $fusionObject); | |||
$cacheMetadata = array_pop($this->cacheMetadata); | |||
$output = $this->contentCache->createDynamicCachedSegment($output, $evaluateContext['fusionPath'], $this->serializeContext($contextVariables), $evaluateContext['cacheIdentifierValues'], $cacheTags, $cacheMetadata['lifetime'], $evaluateContext['cacheDiscriminator']); | |||
$output = $this->contentCache->createDynamicCachedSegment($output, $evaluateContext['fusionPath'], $this->serializeContext($contextVariables), $evaluateContext['cacheIdentifierValues'], $cacheTags, $cacheMetadata['lifetime'], (string)$evaluateContext['cacheDiscriminator']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one might be interesting.. Without this change, the functional tests fail because$evaluateContext['cacheDiscriminator']
is false
at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed:
1) Neos\Fusion\Tests\Functional\FusionObjects\ContentCacheTest::dynamicSegmentCacheBehavesLikeUncachedIfDiscriminatorIsDisabled
TypeError: md5(): Argument #1 ($string) must be of type string, false given
/Cache/Code/Flow_Object_Classes/Neos_Fusion_Core_Cache_ContentCache.php:161
and without this cast we wouldnt support this usecase you wrote a test for ^^
Line 576 in 6ca2ece
entryDiscriminator = ${discriminatorObject.value == 'disable' ? false : discriminatorObject.value} |
@cache.entryDiscriminator = ${false}
@@ -215,7 +217,7 @@ public function deleteElectronicAddressAction(User $user, ElectronicAddress $ele | |||
$this->userService->updateUser($user); | |||
|
|||
$this->addFlashMessage( | |||
$this->translator->translateById('userSettings.electronicAddressRemoved.body', [htmlspecialchars($electronicAddress->getIdentifier()), htmlspecialchars($electronicAddress->getType()), htmlspecialchars($user->getName())], null, null, 'Modules', 'Neos.Neos'), | |||
$this->translator->translateById('userSettings.electronicAddressRemoved.body', [htmlspecialchars($electronicAddress->getIdentifier()), htmlspecialchars($electronicAddress->getType()), htmlspecialchars((string)$user->getName())], null, null, 'Modules', 'Neos.Neos'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was one of the few cases where we relied on implicit casting. With the strict_types
declaration in place this line led to an error
@@ -92,7 +94,7 @@ class XliffService | |||
*/ | |||
public function getCachedJson(Locale $locale): string | |||
{ | |||
$cacheIdentifier = md5($locale); | |||
$cacheIdentifier = md5((string)$locale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another one
It's green, and super straight forward, merging so we get rid of it |
Adds a
declaration to all PHP files of the core that were missing one and implements & defines a new PHPStan rule to enforce them from now on
Related: #5239