Skip to content

[3.0] Fixed warming up custom configs and added support for custom config definition#52

Merged
HeahDude merged 1 commit intomasterfrom
unknown repository
Dec 14, 2019
Merged

[3.0] Fixed warming up custom configs and added support for custom config definition#52
HeahDude merged 1 commit intomasterfrom
unknown repository

Conversation

@HeahDude
Copy link
Copy Markdown
Collaborator

@HeahDude HeahDude commented Jul 29, 2018

Definitely fixes #22, while closing #26 and #34.

  • [BC break] Dropped support for PHP 5.x. PHP 7.1 minimum required.
  • Added a new HTMLPurifierConfigFactory to handle cache properly and to
    build custom html definition from bundle configuration.
  • [BC Break] The bundle configuration has changed
  • Added CHANGELOG and UPGRADE files
  • Refactored SerializerCacheWarmer to preload each profile configuration
  • [BC break] Added type hints and return type hints

TODO:

@HeahDude HeahDude mentioned this pull request Jul 29, 2018
3 tasks
@HeahDude
Copy link
Copy Markdown
Collaborator Author

ping @XWB @stof

@stof
Copy link
Copy Markdown
Contributor

stof commented Nov 23, 2018

As 2.0 has already been tagged, a BC break on the configuration is not really possible anymore.

@spolischook
Copy link
Copy Markdown
Contributor

@stof should we keep this until version 3.0?

@XWB
Copy link
Copy Markdown

XWB commented Nov 23, 2018

@HeahDude Another solution would be adding the new configuration, and deprecating the old one.

@stof
Copy link
Copy Markdown
Contributor

stof commented Nov 23, 2018

@XWB the hard part with that is that the existing config has prototyped node for profiles as the root node itself, making it hard to introduce anything else new.

Btw, that's one of the reason why having a prototyped node as the root is a bad idea (the other reason is that it makes it impossible to support XML config files)

@HeahDude
Copy link
Copy Markdown
Collaborator Author

@XWB, not sure it is worth it, we should think about a 3.0 release instead, if we really want this new config, what I would vote to. I don't think we should add complexity here because we made a tag too soon.

We did such thing in the FrameworkExtraBundle, the 4.0 has never really had the time to be required somewhere. Here some months have passed, but I don't think it's an issue actually.

@jon-ht
Copy link
Copy Markdown

jon-ht commented Apr 30, 2019

Hi,

I'd like to use your PR but I'm facing a problem with cache warmup. It gives me this error

User Warning: Base directory (...)\var\cache\dev/htmlpurifier does not exist, please create or change using %Cache.SerializerPath

image

My config is really simple

exercise_html_purifier:
    html_profiles:
        # full configuration reference: http://htmlpurifier.org/live/configdoc/plain.html
#        default:
#            config:
#                Cache.SerializerPermissions: 777
        notice:
            config:
                HTML.Allowed: 'strong,p[class]'
            elements:
                hide:
                    - Inline
                    - Inline
                    - Common

This error is thrown by this line in ezyang/htmlpurifier

It should be a simple warning but Symfony CacheWarmer breaks and fails to build the rest of cache files. I'm not having any trouble if I use master branch

@bobvandevijver
Copy link
Copy Markdown
Contributor

@HeahDude Is there any progress on this feature? I would really like to use this, instead of overwriting the config factory myself.

@bastos71
Copy link
Copy Markdown

@stof will this PR be merged soon ? More and more we are facing problems regarding rights problems in cache directory, creating issues during deployments and during dev steps ...

@stof
Copy link
Copy Markdown
Contributor

stof commented Oct 16, 2019

@bastos71 no idea. I'm not the maintainer.

@bastos71
Copy link
Copy Markdown

oh sorry, then @spolischook ? or @HeahDude ?

@spolischook
Copy link
Copy Markdown
Contributor

@HeahDude could you please resolve conflicts

@HeahDude
Copy link
Copy Markdown
Collaborator Author

The PR has BC breaks, we need a v3 now that v2 has been tagged. I'll try to update the PR this week end.

@HeahDude
Copy link
Copy Markdown
Collaborator Author

HeahDude commented Dec 7, 2019

Ready for review again (ping @stof), I've fixed @jon-ht bug report. @jon-ht could you confirm please?

@jon-ht
Copy link
Copy Markdown

jon-ht commented Dec 9, 2019

@HeahDude I'm sorry but I'm not on the project using this bundle anymore. But I guess applying my config above might give you an idea on the resolution

@HeahDude HeahDude changed the title [2.0] Fixed warming up custom configs and added support for custom config definition [3.0] Fixed warming up custom configs and added support for custom config definition Dec 12, 2019
@HeahDude HeahDude added this to the 3.0 milestone Dec 12, 2019
@HeahDude
Copy link
Copy Markdown
Collaborator Author

Test are green 🎉, I try to get a better tests coverage then I merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Permissions issue when clearing cache

7 participants