Skip to content
This repository was archived by the owner on Jan 1, 2023. It is now read-only.

Add CacheBustingWorker support v2 #240

Merged
merged 3 commits into from
Nov 19, 2013
Merged

Conversation

sandermarechal
Copy link
Contributor

This is an updated version of #119, rebased on master and updated with regards to the changes in the CacheBustingWorker in assetic.

@stof
Copy link
Member

stof commented Nov 18, 2013

Can you also update the XSD with the new config to support XML configs ?

@@ -175,6 +175,21 @@ public function getConfigTreeBuilder()
->end()
->end()

// workers
Copy link
Contributor

Choose a reason for hiding this comment

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

wonder if all this big tree can be wrapped each block into a method to refactor nicely the sections like elsewhere is done as this is getting too long

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps better in a separate PR?

Choose a reason for hiding this comment

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

@sandermarechal 👍 Finish one task, than the next one.

@sandermarechal
Copy link
Contributor Author

@stof I have updated the XSD

stof added a commit that referenced this pull request Nov 19, 2013
Add CacheBustingWorker support v2
@stof stof merged commit 43cc13f into symfony:master Nov 19, 2013
@Ninir
Copy link

Ninir commented Nov 19, 2013

This is amazing, thanks @sandermarechal @stof
Fixing a lot of issues and dev dependencies ;)

@sandermarechal
Copy link
Contributor Author

@Ninir You're welcome. I will let someone else pick the fight over enabling this by default in a next major release of Symfony. There seemed to be some support for that in #119

@nurikabe
Copy link

I second @Ninir. Thank you very much for this.

@stof
Copy link
Member

stof commented Nov 20, 2013

@sandermarechal No, #119 was not enabling by default.

@sandermarechal
Copy link
Contributor Author

@stof I know, that's why this one isn't either. But there were quite some people giving you a +1 for enabling it by default. That's what I meant.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants