Skip to content

Conversation

@ciastektk
Copy link
Contributor

🎫 Issue IBX-10341

Description:

Fixes bug - accessing property before initialization introduced in #38

For QA:

Documentation:

@sonarqubecloud
Copy link

@ciastektk ciastektk added the Bug Something isn't working label Jul 21, 2025
@ciastektk ciastektk requested a review from a team July 21, 2025 08:26
@ezrobot ezrobot requested review from Steveb-p, ViniTou, adamwojs, alongosz, barw4, konradoboza, mikadamczyk, tbialcz and wiewiurdp and removed request for a team July 21, 2025 08:26
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.

Preferably, in that case, I'd see here some unit test mocking serialization behavior, to see what contents is stored in $this->data.

private function getTemplatePathRegistry(): TemplatePathRegistryInterface
{
return $this->templatePathRegistry;
return $this->templatePathRegistry ??= unserialize($this->data['template_path_registry']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is serializing and unserializing services a common pattern in Symfony Data Collectors?

Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

I understand that this is the current state of code, but really serialization should not be used for Data Collector purposes.

Every data collector I checked was extracting only the data it was capable of displaying in the profiler. Serialization of a whole service instance will bloat the profiler record, especially if registry is decorated.

@adamwojs adamwojs merged commit 111d66d into main Jul 21, 2025
19 checks passed
@adamwojs adamwojs deleted the ibx-10341-fixed-twig-data-collector branch July 21, 2025 10:59
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.

7 participants