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

Fix TraceableFileLocator #894

Merged
merged 3 commits into from
Sep 13, 2022

Conversation

jdreesen
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #858
License MIT

The TraceableFileLocator, which was introduced in #858, to log some data for Symfony's profiler panel, was broken. It implemented a method that doesn't exist anywhere in the project and thus never gets called, so nothing is logged. In the profiler panel you may think everything's alright, though, as the message if this list is empty, probably all the data are cached as expected gets displayed, which is probably even true most of the time.

The new TraceableFileLocator now also decorates the original one, instead of extending it, which makes it more flexible in case someone else wants to decorate the FileLocator, too.

@jdreesen jdreesen marked this pull request as draft June 29, 2022 10:11
@jdreesen jdreesen marked this pull request as ready for review June 29, 2022 10:44
Copy link
Collaborator

@scyzoryck scyzoryck left a comment

Choose a reason for hiding this comment

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

Thank you for contribution! I left one suggestion for more test cleanup.

Best, Marcin

@jdreesen
Copy link
Contributor Author

I think this is ready.

@goetas
Copy link
Collaborator

goetas commented Sep 12, 2022

im sorry if it took me this long to answer, could you please rebase this?

@jdreesen
Copy link
Contributor Author

Sure, done.

I don't think the CI failure is related to this PR. It's the same as in #893.

@jdreesen
Copy link
Contributor Author

Should be fixed by #901

@goetas
Copy link
Collaborator

goetas commented Sep 13, 2022

Can this be considered a duplicate of #900?

@jdreesen
Copy link
Contributor Author

I'm not sure, I tend to no.
@scyzoryck what do you think?

@goetas
Copy link
Collaborator

goetas commented Sep 13, 2022

I'm sorry , now i remember what was this about. i forgot to commit it in the jms/metadata package. it was supposed to be a new feature that allows you to know which files were attempted to be loaded when looking for metadata ( a common problem i saw in many invalid bug reports).

I will adjust it as soon as I can.

Thanks!

@goetas goetas self-assigned this Sep 13, 2022
@goetas goetas merged commit a38c135 into schmittjoh:master Sep 13, 2022
@goetas
Copy link
Collaborator

goetas commented Sep 13, 2022

thanks

@jdreesen jdreesen deleted the fix-profiler-file-locator branch September 13, 2022 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants