Skip to content

[DI] Use require_once instead of require when appending cache warmer-returned files to preload file #36796

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 12, 2020
Merged

[DI] Use require_once instead of require when appending cache warmer-returned files to preload file #36796

merged 1 commit into from
May 12, 2020

Conversation

ovrflo
Copy link
Contributor

@ovrflo ovrflo commented May 12, 2020 •
edited
Loading

Use require_once instead of require when appending cache warmer-returned files to preload file.

Q A
Branch? master/5.1
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #36793
License MIT

As requested in #36793, I have changed the Preloader to append files with require_once (instead of the existing require). This should also help for cases when multiple CacheWarmers return the same file(s).

@ovrflo
bug symfony#36793 [DI][Preload] Use require_once instead of require w…
a53d126 
…hen appending cache warmer-returned files to preload file.
@nicolas-grekas nicolas-grekas added this to the 5.1 milestone May 12, 2020
@nicolas-grekas nicolas-grekas changed the title bug #36793 [DI][Preload] Use require_once instead of require when app… [DI] Use require_once instead of require when appending cache warmer-returned files to preload file May 12, 2020
@nicolas-grekas
Copy link
Member

Thank you @ovrflo.

@nicolas-grekas nicolas-grekas merged commit 869770c into symfony:master May 12, 2020
@stof
Copy link
Member

stof commented May 13, 2020

Couldn't we deduplicate the list instead ?

@ovrflo
Copy link
Contributor Author

ovrflo commented May 13, 2020 •
edited
Loading

Deduplicating the list would be ideal, but I don't see how we could do it fast (as development time, since we need to release soon) and reliable.

Last night I was thinking of a different approach. Opcache wants us to provide one file for preloading. That doesn't mean that that single file can't load additional preloaders, right ? What if we had an entire directory for preloading ? (./var/cache/prod/preload/*.php). This could allow us to write preload files from multiple sources (Kernel, CacheWarmupCommand). We could also generate some sort of .meta file to keep track of already preloaded stuff ? I know that my idea is a bit off-topic (deduplication), but I felt the need to mention it :D

Any thoughts on how we could approach deduplication ?

@nicolas-grekas
Copy link
Member

IMHO, we don't care, require_once is perfect for the job.

@ovrflo ovrflo deleted the fix-preload branch May 13, 2020 13:03
@fabpot fabpot mentioned this pull request May 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

Preload broken with cache:warmup command
4 participants
@ovrflo @nicolas-grekas @stof @carsonbot