Prepare for v2 by HeahDude · Pull Request #46 · Exercise/HTMLPurifierBundle

HeahDude · 2017-12-12T19:27:46Z

Require PHP 5.5 or 7.0 and Symfony 3.4 minimum
Refactored extension and enabled autowiring
[BC Break] Removed classes parameters
[BC Break] Removed the form data transformer
added a new text form type extension with a purifier listener to purify submitted data in all text based fields, using opt-in and custom profile thanks to dedicated options
added a new "exercise.html_purifier" tag to make custom purifier implementations available as profile through form options and Twig filter
added a purifiers registry to lazy load purifiers everywhere
added a Twig HTMLPurifierRuntime for better performances
upgraded the LICENSE and README files

TODO

rebase after Add support for Symfony 4 #47 has been merged

For other PRs:

merge https://github.com/heahprod/HTMLPurifierBundle/pull/2
move classes in a src/ folder and tests in tests/

HeahDude · 2017-12-12T19:34:50Z

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 · 2017-12-12T19:47:09Z

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 · 2017-12-12T20:24:38Z

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

HeahDude · 2017-12-12T20:27:35Z

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

spolischook · 2017-12-12T21:13:25Z

Yep

spolischook · 2017-12-12T21:14:11Z

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

HeahDude · 2017-12-12T22:51:11Z

Should be good now.

spolischook · 2017-12-12T22:54:29Z

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

HeahDude · 2017-12-12T23:02:47Z

With pleasure :)

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

spolischook · 2017-12-12T23:05:29Z

It should be tested with symfony from composer.json

HeahDude · 2017-12-12T23:41:21Z

I confirm tests pass when forcing Symfony 2 requirements.

HeahDude · 2017-12-13T22:17:00Z

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 · 2017-12-16T22:04:57Z

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

HeahDude · 2017-12-17T03:28:13Z

This is the case.

HeahDude · 2017-12-17T15:41:23Z

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.

nicolas-grekas

I would recommend to not bump PHP to 7.1 because it would prevent Symfony 3.4+PHP 5 users from using the bundle.

nicolas-grekas · 2018-01-03T13:13:07Z

+        $locator = $container->getDefinition('exercise_html_purifier.purifiers_locator');
+
+        foreach ($container->findTaggedServiceIds('exercise.html_purifier') as $id => $tags) {
+            foreach ($tags as $tag) {


replace foreach by $tags[0]

nicolas-grekas · 2018-01-03T13:21:11Z

+    private $cachedListeners;
+    private $purifiersRegistry;
+
+    public function __construct(HTMLPurifiersRegistry $registry)


the type hint should be against an interface

nicolas-grekas · 2018-01-03T13:25:02Z

+
+        $purifiers = [];
+        $purifiersLocator = $container
+            ->register('exercise_html_purifier.purifiers_locator', ServiceLocator::class)


you should use ServiceLocatorTagPass::register() instead (with 3rd arg)

nicolas-grekas · 2018-01-03T13:30:58Z

+{
+    private $purifier;
+
+    public function __construct(\HTMLPurifier $purifier)


this should be lazy: inject two args: a registry + $profile

spolischook · 2018-01-04T10:32:47Z

Thanks @nicolas-grekas for review!

yakobe · 2018-02-02T20:14:29Z

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 · 2018-02-03T03:25:22Z

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

alister · 2018-02-19T22:57:50Z

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 · 2018-02-20T10:24:36Z

@alister see #47 for the Symfony 4 support.

stof · 2018-02-20T10:28:09Z

+            $container->setAlias('exercise_html_purifier.'.$tags[0]['profile'], $id);
+        }
+
+        $locator->setArguments($purifiers);


altering a ServiceLocator service is a bad idea, as the ServiceLocatorPass performs de-duplication of the service locators based on their config. You should rather ask the ServiceLocatorPass to register the service locator for the new set of arguments (or even better, configure the service locator only here in the compiler pass instead of doing it first in the DI extension and then here to find additional profiles)

Fixed, thanks!

stof · 2018-02-20T10:31:06Z

+    private function getHTMLPurifierListener($profile)
+    {
+        if (isset($this->cachedListeners[$profile])) {
+            return $this->cachedListeners[$profile];


this caching is useless IMO. The instantiation of the listener is cheap, and such memoization can lead to memory leaks in long-running processes.

stof · 2018-02-20T10:31:25Z

+     */
+    private function getPurifier()
+    {
+        if (null !== $this->cachedPurifier) {


this memoization is not worth it IMO

stof · 2018-02-20T10:32:40Z

+    #...
+
+    exercise_html_purifier.custom:
+        alias: \HTMLPurifier


that broken. You need to configure a HTMLPurifier alias targetting exercise_html_purifier.custom, not the opposite

Fixed, plus added alternative syntax.

stof · 2018-02-20T10:34:05Z

        }

-        $container->setParameter('exercise_html_purifier.cache_warmer.serializer.paths', array_unique($paths));
+        $container->register(HTMLPurifiersRegistry::class)


the registry should not be registered using its class name, but using an id instead. We don't want the HTMLPurifiersRegistry to be available for autowiring, only the interface.

stof · 2018-02-20T10:35:13Z

-            <argument>%exercise_html_purifier.cache_warmer.serializer.paths%</argument>
-            <argument type="service" id="exercise_html_purifier.default" />
-            <tag name="kernel.cache_warmer" />
+        <defaults public="false" autowire="true" autoconfigure="true" />


For shared bundles, using explicit configuration (rather than autowiring and autoconfiguration) is better, as it ensures that the bundle maintainer knows how the bundle gets configured.

stof · 2018-03-09T11:07:27Z

[BC Break] Removed the form data transformer

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

XWB · 2018-06-13T10:39:40Z

@HeahDude Any updates on this PR?

HeahDude · 2018-07-22T11:48:42Z

@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 · 2018-07-22T13:15:48Z

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

XWB · 2018-07-22T16:42:57Z

@stof Can you have a look as well?

stof · 2018-07-23T10:54:23Z

+            ->setPublic(false)
+        ;
+        $container->setAlias(HTMLPurifiersRegistryInterface::class, 'exercise_html_purifier.purifiers_registry');
+        $container->setAlias(\HTMLPurifier::class, 'exercise_html_purifier.default');


autoloading aliases should always be private

stof · 2018-07-23T10:56:57Z

+            return $this->purifiersLocator->get($profile);
+        }
+
+        throw new \InvalidArgumentException(sprintf('The profile "%s" is not registered.', $profile));


if you let $this->purifiersLocator->get trigger an exception instead, it might suggest alternative names.

Ha, didn't know that thanks. Fixed.

stof · 2018-07-23T10:58:42Z

  [configuration documentation]: http://htmlpurifier.org/live/configdoc/plain.html

-## Cache Warming ##
+## Configuration in Symfony 4


isn't this exactly the same than for Symfony 3, except for the path to the YAML file, which is not actually related to 3 vs 4 but to Flex vs non-Flex (you can use Flex with 3.4)

Right, fixed.

stof · 2018-07-23T10:59:56Z

+
+## Purifiers Registry
+
+A `Heah\HtmlPurifierBundle\HtmlPurifiersRegistry` class is registered by default


wrong namespace

Good catch! Fixed.

stof · 2018-07-23T11:02:13Z

+
+class HTMLPurifierRuntime implements RuntimeExtensionInterface
+{
+    private $purifiersRegistry = [];


initializing it to an empty array is wrong. This variable is not meant to contain an array

Artefact of previous refactoring, good catch again, thanks! Fixed.

stof · 2018-07-23T11:02:49Z

-    "description": "HTMLPurifier integration for your Symfony2 project",
-    "keywords": ["htmlpurifier"],
+    "description": "HTMLPurifier integration for your Symfony project",
+    "keywords": ["htmlpurifier", "html", "purifier", "symfony3", "symfony4"],


keyword should be symfony, not symfony3 or symfony4

stof · 2018-07-23T11:03:26Z

-        "twig/twig": "~1.3|~2.0"
+        "phpunit/phpunit": "^6.5.4",
+        "symfony/form": "~3.4.1 || ^4.0.1",
+        "twig/twig": "~1.6.5 || ~2.0"


runtimes are much newer than 1.6.5 IIRC. So this might be the wrong lowest bound.

Runtime have been introduced in 1.26 twigphp/Twig@9cb6860#diff-16e3dbccf13c8c447eff98d226c5d89bR813.
But this is more about namespaces, so I changed to 1.34|2.4. Thanks for the hint.

stof · 2018-07-23T11:03:40Z

-        "php": ">=5.3.2",
-        "symfony/framework-bundle": "~2.0|~3.0",
+        "php": "^5.5.9|>=7.0.8",
+        "symfony/framework-bundle": "~3.4.1 || ^4.0.1",


does it actually depend on FrameworkBundle ? Or only on some other components ?

Hmm, good question. I guess the http kernel could be added with the others require-dev deps for the cache warmer, keeping only the DI in the required ones. WDYT?

I've done that.

stof · 2018-07-23T11:04:33Z

    "extra": {
        "branch-alias": {
-            "dev-master": "1.0.x-dev"
+            "dev-master": "4.0.x-dev"


this change looks wrong to me. Why making it a 4.0 release ? This would be a 2.0 one at most

A request from the maintainer in one of the first comment of this PR.

I prefer to be consistent with symfony version

but the bundle supports both 3.x and 4.x. So synchronized version number does not make sense. Thus, it means you restrict yourselves to do major versions every 2 years when Symfony does them, instead of doing them when you need them.

even official Symfony bundles developed in separate repos (MonologBundle, SwiftmailerBundle, MakerBundle) have separate version numbers (3.3.0, 3.2.2, 1.5.0 respectively).

HeahDude · 2018-07-23T22:04:32Z

Thanks again @stof. Comments addressed.

javiereguiluz · 2018-07-24T06:39:54Z

+
+        $purifiers = [];
+
+        foreach ($container->findTaggedServiceIds('exercise.html_purifier') as $id => $tags) {


Here (and in the error message) you could use the PURIFIER_TAG constant instead of the exercise.html_purifier string, right?

javiereguiluz · 2018-07-24T06:43:13Z

@@ -0,0 +1,68 @@
+<?php


Blank line here after <?php

HeahDude · 2018-07-24T07:17:33Z

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

XWB · 2018-07-24T09:19:59Z

+        try {
+            $registry = $container->findDefinition(HTMLPurifiersRegistryInterface::class);
+        } catch (ServiceNotFoundException $e) {
+            return;


Instead of the try/catch block, why not something like this:

if (!$container->hasDefinition(HTMLPurifiersRegistryInterface::class)) { return; }

There won't be any definition and the check will fail, we need to check for an alias here.

XWB · 2018-07-24T09:21:56Z

+            $purifier = $container->getDefinition($id);
+
+            if (empty($purifier->getArguments())) {
+                $configId = "exercise_html_purifier.config.$profile";


I'm not sure if this bundle follows Symfony style. If so, it should be 'exercise_html_purifier.config'.$profile;.

This syntax may be used in the core, it mainly depends on the contextual readability it provides versus perf. This is more performant like this :)

XWB · 2018-07-24T09:22:27Z

-                        ->addMethodCall('loadArray', array($config));
-                }
+        foreach ($configs as $name => $config) {
+            $configId = "exercise_html_purifier.config.$name";


I'm not sure if this bundle follows Symfony style. If so, it should be 'exercise_html_purifier.config'.$name;.

XWB · 2018-07-24T09:22:55Z

-                'exercise_html_purifier.' . $name,
-                new Definition('%exercise_html_purifier.class%', array(new Reference($configId)))
-            );
+            $container->register("exercise_html_purifier.$name", \HTMLPurifier::class)


Same question as above :)

XWB · 2018-07-24T09:25:29Z


 class ExerciseHTMLPurifierBundle extends Bundle
 {
+    public function build(ContainerBuilder $container)


@inheritdoc is missing.

XWB · 2018-07-24T09:25:59Z

+        $this->profile = $profile;
+    }
+
+    public function purifySubmittedData(FormEvent $event)


Method doc is missing.

Following Symfony style here, we don't need to add : void.

XWB · 2018-07-24T09:26:28Z

+{
+    private $purifiersRegistry;
+
+    public function __construct(HTMLPurifiersRegistryInterface $registry)


Missing docs.

What would be the benefit?

XWB · 2018-07-24T09:26:48Z

+        $this->purifiersRegistry = $registry;
+    }
+
+    public function getExtendedType()


The entire class is missing @inheritdoc comments.

XWB · 2018-07-24T09:27:06Z

+{
+    private $purifiersLocator;
+
+    public function __construct(ContainerInterface $purifiersLocator)


Missing docs.

which doc would be needed here ? Only duplicating the typehint would be useless

Not sure it would be valuable here.

I would swear Symfony did something similar. Apparently not.

HeahDude · 2018-07-24T19:28:10Z

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

HeahDude · 2018-07-27T19:58:22Z

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

HeahDude · 2018-07-27T20:00:58Z

+    }
+
+    /**
+     * @inheritdoc}


XWB · 2018-07-27T20:18:08Z

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

spolischook · 2018-07-28T04:34:52Z

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

HeahDude · 2018-07-28T10:24:42Z

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

HeahDude · 2018-07-28T10:54:54Z

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.

- Require PHP 5.5 or 7.0 and Symfony 3.4 minimum - Refactored extension and enabled autowiring - [BC Break] Removed classes parameters - [BC Break] Removed the form data transformer - added a new text form type extension with a purifier listener to purify submitted data in all text based fields, using opt-in and custom profile thanks to dedicated options - added a new "exercise.html_purifier" tag to make custom purifier implementations available as profile through form options and Twig filter - added a purifiers registry to lazy load purifiers everywhere - added a Twig HTMLPurifierRuntime for better performances - upgraded the LICENSE and README files

HeahDude · 2018-07-28T13:54:57Z

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

HeahDude · 2018-07-28T18:18:59Z

@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 · 2018-07-29T19:03:06Z

Thanks @HeahDude for your contribution

HeahDude · 2018-07-29T20:03:54Z

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

HeahDude closed this Dec 12, 2017

spolischook reopened this Dec 12, 2017

HeahDude changed the base branch from master to 4.0 December 12, 2017 22:44

spolischook changed the title ~~Upgraded to v1~~ Upgraded to v4 Dec 12, 2017

HeahDude changed the title ~~Upgraded to v4~~ [WIP] Upgraded to v4 Dec 14, 2017

This was referenced Dec 17, 2017

how to add custom attribute to whitelist by edit config.yml #26

Closed

Permissions issue when clearing cache #22

Closed

nicolas-grekas reviewed Jan 3, 2018

View reviewed changes

stof reviewed Feb 20, 2018

View reviewed changes

stof reviewed Jul 23, 2018

View reviewed changes

HeahDude changed the title ~~[WIP] Upgraded to v4~~ Prepare for v4 Jul 23, 2018

javiereguiluz reviewed Jul 24, 2018

View reviewed changes

XWB suggested changes Jul 24, 2018

View reviewed changes

XWB approved these changes Jul 25, 2018

View reviewed changes

HeahDude commented Jul 27, 2018

View reviewed changes

Comment thread Form/Listener/HTMLPurifierListener.php Outdated

}

/**

* @inheritdoc}

Copy link
Copy Markdown

Collaborator Author

HeahDude Jul 27, 2018

Choose a reason for hiding this comment

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

CS

HeahDude changed the title ~~Prepare for v4~~ Prepare for v2 Jul 28, 2018

HeahDude changed the base branch from 4.0 to master July 28, 2018 10:52

spolischook merged commit 728672b into Exercise:master Jul 29, 2018


		## Purifiers Registry

		A `Heah\HtmlPurifierBundle\HtmlPurifiersRegistry` class is registered by default


		$purifiers = [];

		foreach ($container->findTaggedServiceIds('exercise.html_purifier') as $id => $tags) {

Conversation

HeahDude commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HeahDude commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HeahDude commented Dec 12, 2017

Uh oh!

spolischook commented Dec 12, 2017

Uh oh!

HeahDude commented Dec 12, 2017

Uh oh!

spolischook commented Dec 12, 2017

Uh oh!

spolischook commented Dec 12, 2017

Uh oh!

HeahDude commented Dec 12, 2017

Uh oh!

spolischook commented Dec 12, 2017

Uh oh!

HeahDude commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spolischook commented Dec 12, 2017

Uh oh!

HeahDude commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HeahDude commented Dec 13, 2017

Uh oh!

spolischook commented Dec 16, 2017

Uh oh!

HeahDude commented Dec 17, 2017

Uh oh!

HeahDude commented Dec 17, 2017

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spolischook commented Jan 4, 2018

Uh oh!

yakobe commented Feb 2, 2018

Uh oh!

spolischook commented Feb 3, 2018

Uh oh!

alister commented Feb 19, 2018

Uh oh!

stof commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeahDude commented Dec 12, 2017 •

edited

Loading

HeahDude commented Dec 12, 2017 •

edited

Loading

HeahDude commented Dec 12, 2017 •

edited

Loading

HeahDude commented Dec 12, 2017 •

edited

Loading