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

Added setter for HTMLPurifier config in HTML writer #2383

Merged
merged 15 commits into from
Nov 23, 2021
Merged

Added setter for HTMLPurifier config in HTML writer #2383

merged 15 commits into from
Nov 23, 2021

Conversation

vladdnepr
Copy link
Contributor

This is:

- [ ] a bugfix
- [* ] a new feature

Checklist:

Why this change is needed?

Sometimes default config values of HTMLPurifier are not right.

For example default cache path in prod env is not writeable. But we have app cache folder, where we can write cache data.

Снимок экрана 2021-11-09 в 11 53 56

Now we can set custom config and change what we need.

@vladdnepr
Copy link
Contributor Author

@MarkBaker @PowerKiKi hi. Can somebody merge this PR?

@oleibman
Copy link
Collaborator

This seems unnecessarily complicated; also, if the default code potentially writes to a protected directory, as your explanation makes clear that it might, we are better off solving that problem for everyone:

            $sanitizer = new HTMLPurifier();
            $cachePath = File::sysGetTempDir() . '/phpsppur';
            if (is_dir($cachePath) || mkdir($cachePath)) {
                $sanitizer->config->set('Cache.SerializerPath', $cachePath);
            }

We use a similar approach for Mpdf. I think this would solve your problem without the need for changes at the application end, and without the need for extra properties and methods and documentation.

Use tmp dir in HTMLPurifier
Use tmp dir in HTMLPurifier
Use tmp dir in HTMLPurifier
Use tmp dir in HTMLPurifier
Use tmp dir in HTMLPurifier
@vladdnepr
Copy link
Contributor Author

This seems unnecessarily complicated; also, if the default code potentially writes to a protected directory, as your explanation makes clear that it might, we are better off solving that problem for everyone:

            $sanitizer = new HTMLPurifier();
            $cachePath = File::sysGetTempDir() . '/phpsppur';
            if (is_dir($cachePath) || mkdir($cachePath)) {
                $sanitizer->config->set('Cache.SerializerPath', $cachePath);
            }

We use a similar approach for Mpdf. I think this would solve your problem without the need for changes at the application end, and without the need for extra properties and methods and documentation.

Done. Please check

@oleibman
Copy link
Collaborator

Getting closer, but ... I would prefer that you not mess around with the PDF code. It works just fine now, and has nothing to do with your change. There is no reason to think PDF and Purifier use, and always will use, the temporary directory in compatible manners. Html does not have a tempdir because it almost never needs one; any spreadsheet lacking comments will still have to test the existence of and possibly allocate a new directory that it will not use, because of the code you've added to the constructor.

@vladdnepr
Copy link
Contributor Author

Getting closer, but ... I would prefer that you not mess around with the PDF code. It works just fine now, and has nothing to do with your change. There is no reason to think PDF and Purifier use, and always will use, the temporary directory in compatible manners. Html does not have a tempdir because it almost never needs one; any spreadsheet lacking comments will still have to test the existence of and possibly allocate a new directory that it will not use, because of the code you've added to the constructor.

Done, please check

@oleibman
Copy link
Collaborator

Thank you. That looks good. I will merge it some time over the next few days.

@PowerKiKi PowerKiKi merged commit 9dcfd9a into PHPOffice:master Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants