Exercise / HTMLPurifierBundle

HTML Purifier is a standards-compliant HTML filter library written in PHP.
http://htmlpurifier.org/
Other
275 stars 56 forks source link

Prepare for v2 #46

Closed HeahDude closed 6 years ago

HeahDude commented 6 years ago

TODO


For other PRs:

HeahDude commented 6 years ago

Sorry for the noise I wanted to open a PR on my own fork to test the support of some Symfony 4 projects needing this dependency.

Thank you.

HeahDude commented 6 years ago

And btw if you're still interested in maintaining this bundle and any feature in there I would be glad to open a proper PR and add a BC layer if needed.

spolischook commented 6 years ago

@HeahDude we need to made some branch for that. Please make PR to 4.0 branch

HeahDude commented 6 years ago

:) So what do you need? I just remove the file headers and submit it?

spolischook commented 6 years ago

Yep

spolischook commented 6 years ago

Please check the tests locally after update - it must be green

HeahDude commented 6 years ago

Should be good now.

spolischook commented 6 years ago

@HeahDude many thanks for your contribution, I'll review you PR and let you know about results

HeahDude commented 6 years ago

With pleasure :)

I'm not sure about the Symfony 2 compatibility, I have no project to test that right now.

spolischook commented 6 years ago

It should be tested with symfony from composer.json

HeahDude commented 6 years ago

I confirm tests pass when forcing Symfony 2 requirements.

HeahDude commented 6 years ago

I fixed a few things about class name convention, some typos here and there, and enforced lazy loading while keeping BC with Symfony 2, that I had left around.

A review would be very welcome :). Thanks!

spolischook commented 6 years ago

Please add phpunit as dev dependency to composer.json, and check that tests is passed

HeahDude commented 6 years ago

This is the case.

HeahDude commented 6 years ago

I've removed a form option about dynamic configs that do not play well with cache.

I've opened https://github.com/heahprod/HTMLPurifierBundle/pull/2 separately to ease the review but it should be merge before release as it breaks BC. It fixes #22 with custom profiles and #26.

Support for Symfony 2 has been drop to ease the code and rely on powerful features. So we should merge #44 to keep BC and full support of 0.2.x versions with Symfony 3.4 and 4.

I've updated the description.

spolischook commented 6 years ago

Thanks @nicolas-grekas for review!

yakobe commented 6 years ago

It this PR ready to be merged? It's one of the last things preventing a symfony 4 upgrade for us. Is there any way I can help out?

spolischook commented 6 years ago

@yakobe can you try it manually and prove that it's works?

alister commented 6 years ago

I've tested #44 with a Composer repository stanza (it works as expected, and the public service works), but I think that this PR does too much. If there was one PR to 'make this work with Symfony 4' (ie: only to make the service public), I'd take another look at it, and then I'd also look at other PRs for the additional functionality.

stof commented 6 years ago

@alister see #47 for the Symfony 4 support.

stof commented 6 years ago

[BC Break] Removed the form data transformer

Why removing the data transformer ? This is a good way to perform HTML filtering.

XWB commented 6 years ago

@HeahDude Any updates on this PR?

HeahDude commented 6 years ago

@XWB updated :).

@stof Thank you very much for the review.

Why removing the data transformer ? This is a good way to perform HTML filtering.

No... definitely not, the transformer is a wrong usage. Implementing the interface means changing the representation, one way to another. As long as an implementation cannot verify:

// Given $a
$b = $transformer->transform($a);

$a === $transformer->reverseTransform($b); // must be true

then this is either wrong or a hack (even if we hack Symfony, though I'm very confident I'll get rid of that for 5.0).

A view transformer is called three times in a submission process by the way.

Here we just want to normalize the submitted data, we don't want to alter model or norm data in any way. Another reason is that now the purify option superseed the default trim processing. Bref, it's much much cleaner like this IMO.

Anyone to test/review it? Thanks!

HeahDude commented 6 years ago

https://github.com/heahprod/HTMLPurifierBundle/pull/2 has been rebased and fixed too.

XWB commented 6 years ago

@stof Can you have a look as well?

HeahDude commented 6 years ago

Thanks again @stof. Comments addressed.

HeahDude commented 6 years ago

@javiereguiluz All fixed, nice catch! Thank you for your review :)

HeahDude commented 6 years ago

Last comments addressed, I'll squash tomorrow. Thank you all for your reviews :).

HeahDude commented 6 years ago

So should I make it a 2.0 before squash? ping @spolischook

XWB commented 6 years ago

IMO it should be 2.0, this PR supports both Symfony 3 and 4 anyway.

spolischook commented 6 years ago

@HeahDude as mentioned stof and XWB lets go with 2.0 version. Thanks for your work!

HeahDude commented 6 years ago

@spolischook Would you create a 1.x branch from master so I can make master the target here?

HeahDude commented 6 years ago

This is now ready to merge from my side. Once done I'll reopen https://github.com/heahprod/HTMLPurifierBundle/pull/2 on master in this repo.

HeahDude commented 6 years ago

@spolischook rebased. Tests are green. Let's merge?

HeahDude commented 6 years ago

@spolischook heahprod#2 is rebased with green tests, and ready to be open on master after merge before we can review, merge and release v2 :).

spolischook commented 6 years ago

Thanks @HeahDude for your contribution

HeahDude commented 6 years ago

Thank you for merging! Here's the following #52 :).