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

reset css rules when new css is set #87

Closed
wants to merge 4 commits into from

Conversation

mente
Copy link

@mente mente commented Nov 6, 2014

Hello,

This PR fixes issue when reusing same instance of CssToInlineStyles class for inlining, e.g. in bulk emails option. Also it speeds up inlining css a bit for bulk emails by parsing css only once.

Regards,
Alex

@stof
Copy link
Collaborator

stof commented Nov 6, 2014

Not reprocessing the rules is broken for inline style tags

@mente
Copy link
Author

mente commented Nov 6, 2014

@stof why do you think so? Rules will be reprocessed as soon as new css is set

@mente
Copy link
Author

mente commented Nov 6, 2014

Inline css tags are exactly where I came from. Sending bulk email from console was a big pain. Template compilation started from 0.1sec and increased to 8sec after 1000 emails

@stof
Copy link
Collaborator

stof commented Nov 6, 2014

Calling convert() with an HTML template using style tags will not call setCss, and so will not reprocess the CSS. Are you sure that you are processing style tags ? This feature is off by default.

@mente
Copy link
Author

mente commented Nov 6, 2014

Ok, now I see. Will provide fix shortly

@barryvdh
Copy link
Contributor

barryvdh commented Nov 7, 2014

Can't you just process your template once and replace the name/email etc? Like with the Decorator plugin in Swiftmailer: http://swiftmailer.org/docs/plugins.html#decorator-plugin

@mente
Copy link
Author

mente commented Nov 7, 2014

@barryvdh I'm using symfony bundle that provides twig integration with CssToInlineStyles.

There are multiple ways of solving my issue but as for me - there should be no problem with using same instance for multiple templates. Or at least say somewhere that reusing object is not safe. But if it's easy to support object reusage then why not?

@Hikariii
Copy link
Contributor

I think caching of css rules is better done through injection of a separate object containing the cache.
This will increase flexibility and will avoid potential memory issues like #105.

Even better, the convert() method could be separated internally in a css rule parsing method and an html parsing method. The parsed css could then be cached externally and injected in the html parsing method when needed.

I would even opt to not set the html and the css as properties of the CssToInlineStyles object, but to inject them in the convert method. I think this would greatly improve the flexibility of the class.

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.

None yet

5 participants