Skip to content
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

BUGFIX: #3303 Flush reflection cache for removed php classes #3383

Open
wants to merge 2 commits into
base: 9.0
Choose a base branch
from

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Jul 27, 2024

resolves #3303

There are a few layers to this bugfix.

  1. Previously we ignored deleted files (see file_exists) and did not flush the Flow_Reflection_Status cache accordingly as we are not able to read the namespace from a deleted php file. Now we do a best effort reverse lookup by going through all psr-4 autoload configurations and calculate a namespace under which composer would have found this file.

  2. We introduce a state removedReflectionDataClasses like updatedReflectionData which will be used to check if we have to update the ReflectionData cache.

Upgrade instructions

Review instructions

How to test

We want to test getAllImplementationClassNamesForInterface as by the bugreport. And we can do this simply because we use it at a few places.

  1. Open the Neos.Demo site (in development context)
  2. Remove a not-so-important FlowQuery operation like ReferencePropertyOperation (which is not used on the homepage)
  3. Reload the homepage
  4. Expect everything be ok

Without this fix there will be an error thrown:

Exception in line 58 of Data/Temporary/Development/Cache/Code/Flow_Object_Classes/Neos_Eel_FlowQuery_OperationResolver.php: Class "Neos\ContentRepository\NodeAccess\FlowQueryOperations\ReferencePropertyOperation" not found

69 Neos\Eel\FlowQuery\OperationResolver_Original::buildOperationsAndFinalOperationNames(Neos\Flow\ObjectManagement\ObjectManager)
68 Neos\Eel\FlowQuery\OperationResolver_Original::initializeObject(1)

How the fix works

With the fix applied we get the information that "/Users/neos/Packages/Neos/Neos.ContentRepository.NodeAccess/Classes/FlowQueryOperations/ReferencePropertyOperation.php" was removed. Then we can resolve that it belongs to the package Neos.ContentRepository.NodeAccess and reverse calculate its namespace/cache identifier (with underscores): Neos_ContentRepository_NodeAccess_FlowQueryOperations_ReferencePropertyOperation. Then we can flush the cache accordingly.

Todo

  • check if this is symlink safe! Like for user package in DistributionPackages

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

There are a few layers to this bugfix.

1. Previously we ignored deleted files (see `file_exists`) and did not flush the `Flow_Reflection_Status` cache accordingly as we are not able to read the namespace from a deleted php file.
Now we do a best effort reverse lookup by going through all psr-4 autoload configurations and calculate a namespace under which composer would have found this file.

2. We introduce a state `removedReflectionDataClasses` like `updatedReflectionData` which will be used to check if we have to update the `ReflectionData` cache.
continue;
}
$classNameWithUnderscores = str_replace('\\', '_', $className);
} else {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically we can discuss if we always want to use the psr-4 reverse lookup instead of parsing the file via PhpAnalyzer ... but for that we would need to know how stable this logic is... for example regarding symlinks

@kitsunet
Copy link
Member

I am quite opposed to adding more places where we lookup package classes/class loading to do magic stuff, I think we need to get rid of that concept.
But more importantly, this screams performance bottleneck with what it does (although I just looked at the code as is and this just adds to what is already there). I think the md5 thing was nicer overall. Probably we should streamline this all to be independent from looking into the class file...

@@ -1833,6 +1837,8 @@ protected function forgetClass($className): void

unset($this->classReflectionData[$className]);
unset($this->classesCurrentlyBeingForgotten[$className]);

$this->removedReflectionDataClasses[$className] = true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo check if forgetClass is also called in other occasions and if this flag makes sense in that case.

@@ -2092,7 +2098,7 @@ protected function saveProductionData(): void
*/
protected function updateReflectionData(): void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$this->removedReflectionDataClasses should be reset to [] in here, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Reflection cache for class is not flushed on file deletion
3 participants