Skip to content

[TwigBundle] Cache Warmer node_modules bug fixed #34321

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

Closed
wants to merge 1 commit into from
Closed

[TwigBundle] Cache Warmer node_modules bug fixed #34321

wants to merge 1 commit into from

Conversation

cesurapp
Copy link
Contributor

Q A
Branch? 4.3 bug fixes
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

I created a theme structure in the "templates" directory. A different webpack structure is used for each theme.

I get an error while clearing the Symfony cache "bin/console cache:clear". While the cache is warming up, twig is trying to read files in node_modules directory.

There is no problem with "bin/console cache:clear --no-warmup".

Error:

PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 815104 bytes) in /Users/apaydin/www/emlakpro/apps/vendor/twig/twig/src/Compiler.php on line 129

@cesurapp cesurapp changed the title Cache Warmer node_modules bug fixed [TwigBundle] Cache Warmer node_modules bug fixed Nov 11, 2019
@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Nov 12, 2019
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

The name is specific enough, I think we can do it.

Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

I'm 👎 on this. I don't think this is the right approach (there are a lot of well known directories that could be applied here) and I'm pretty sure you can solve this problem by moving the folder out of templates/ in the meantime.

The right approach in my opinion would be adding a new option to exclude directories (e.g. bundles/ by default) and/or files, using some kind of glob/pattern.

See dicussion about this topic here #34142

@nicolas-grekas
Copy link
Member

there are a lot of well known directories that could be applied here

I'm not sure about this - I mean: node_modules is very very well-known. I can't recall any other directory that well known. That makes it special to me.

I'm pretty sure you can solve this problem by moving the folder out of templates/ in the meantime.

@appaydin please tell us why you didn't do this in the first place?

The right approach in my opinion would be adding a new option to exclude directories (e.g. bundles/ by default) and/or files, using some kind of glob/pattern.

That's a lot of additional complexity for a rare case. Excluding just node_modules would save us from that complexity. That's a significant benefit.

#34142 is far from being solved at all: "non-template files" is undefined. I wouldn't consider this a viable alternative solution in the near term...

@yceruto
Copy link
Member

yceruto commented Nov 16, 2019

I agree, I would also consider this a rare case, that's why I'm against to add more directories arbitrarily to the hardcoded exclusion list and go ahead with workarounds.

I still don't think this use case (having the node_modules/ inside the Twig's templates/ directory) is enough legit and common to exclude it.

@yceruto
Copy link
Member

yceruto commented Nov 16, 2019

Although I don't feel comfortable telling people where they should put their directories/files due to a problem generated by the framework. That's why also I would want do something about it.

@cesurapp
Copy link
Contributor Author

Keeping the front-end of the site in a single directory does not disturb the theme structure. There are different themes created with Vue, and you cannot move the node_modules directory to a different location.

In order to get node_modules into a different directory, we have to split the theme structure into two.

Alternative options are possible, but the directory structure becomes more complex.

https://stackoverflow.com/questions/21818701/can-a-custom-directory-name-be-used-instead-of-node-modules-when-installing-no

@yceruto
Copy link
Member

yceruto commented Nov 16, 2019

The right approach in my opinion would be adding a new option to exclude directories (e.g. bundles/ by default) and/or files, using some kind of glob/pattern.

That's a lot of additional complexity for a rare case. Excluding just node_modules would save us from that complexity. That's a significant benefit.

Please, let's discuss it here #34416, I don't think there's much additional complexity to it.

@fabpot
Copy link
Member

fabpot commented Nov 17, 2019

👎 for me

@nicolas-grekas
Copy link
Member

Closing then, thanks for proposing @appaydin

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.

5 participants