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

IBX-4000: Changed the method name creation for logCall in callable function #410

Merged
merged 3 commits into from
Aug 1, 2024

Conversation

mateuszdebinski
Copy link
Contributor

@mateuszdebinski mateuszdebinski commented Jul 25, 2024

🎫 Issue IBX-4000

Related PRs:

Description:

The problem is that the Ibexa\Bundle\Debug\Collector\PersistenceCacheCollector::getCalls function expects us to format the method as Namespace::Function. With callable functions, METHOD returns Namespace{closure} instead of the correct value. To prevent this, we must use CLASS in these functions and add the method name manually, which will allow us to correctly save the call function to log.

For QA:

Documentation:

@mateuszdebinski mateuszdebinski added Bug Something isn't working Ready for review labels Jul 25, 2024
@mateuszdebinski mateuszdebinski requested a review from a team July 25, 2024 12:09
@mateuszdebinski mateuszdebinski self-assigned this Jul 25, 2024
@mateuszdebinski mateuszdebinski changed the title IBX-4000: Changed the method name creation for logCall in callable fu… IBX-4000: Changed the method name creation for logCall in callable function Jul 25, 2024
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

@mateuszdebinski could you please describe what was the issue here, so everyone can understand what was changed and why?

Also, what could be done so such easy to miss mistake won't happen in any future implementations?

@mateuszdebinski
Copy link
Contributor Author

@alongosz The problem is that the Ibexa\Bundle\Debug\Collector\PersistenceCacheCollector::getCalls function expects us to format the method as Namespace::Function. With callable functions, __METHOD__ returns Namespace\{closure} instead of the correct value. To prevent this, we must use __CLASS__ in these functions and add the method name manually, which will allow us to correctly save the call function to log.

@alongosz
Copy link
Member

@alongosz The problem is that the Ibexa\Bundle\Debug\Collector\PersistenceCacheCollector::getCalls function expects us to format the method as Namespace::Function. With callable functions, __METHOD__ returns Namespace\{closure} instead of the correct value. To prevent this, we must use __CLASS__ in these functions and add the method name manually, which will allow us to correctly save the call function to log.

This is something which should land in the PR description ;-)

That doesn't answer however section question - what can be done to ensure all implementations reference a correct method?

@mateuszdebinski mateuszdebinski requested review from alongosz and a team July 26, 2024 14:31
@konradoboza konradoboza requested a review from a team July 29, 2024 06:21
@@ -219,7 +219,10 @@ final public function testLoadMethodsCacheMiss(
$cacheItem = $this->getCacheItem($key, null);
$handlerMethodName = $this->getHandlerMethodName();

$this->loggerMock->expects($this->once())->method('logCall');
$handler = $this->persistenceCacheHandler->$handlerMethodName();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the handler acquisition moved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently use this variable in the test
->with(get_class($handler) . '::' . $method, self::isType('array'));

tests/lib/Persistence/Cache/AbstractCacheHandlerTest.php Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Jul 29, 2024

@alongosz alongosz merged commit 8a676dc into 4.6 Aug 1, 2024
23 checks passed
@alongosz alongosz deleted the IBX-4000_cache_issue_with_symfony_profiler branch August 1, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working QA approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants