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

Filters::spacelessHtml() Add support for use <script> as link to JS #196

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

janbarasek
Copy link

  • bug fix
  • BC break? yes

In case of block y generate too long code:

{block x|strip}
	Lorem ipsum dolor sit amet,
	<script src="main.js"></script>
	consectetur adipiscing elit.
{/block}

------

{block y|strip}
	Lorem ipsum dolor sit amet,
	<script src="main.js"
			title="value"
	></script>
	consectetur adipiscing elit.
{/block}

Old return:

Lorem ipsum dolor sit amet, <script src="main.js"></script> consectetur adipiscing elit.
------

Lorem ipsum dolor sit amet, <script src="main.js"
			title="value"
	></script> consectetur adipiscing elit.

New return:

Lorem ipsum dolor sit amet, <script src="main.js"></script> consectetur adipiscing elit.
------

Lorem ipsum dolor sit amet, <script src="main.js" title="value"></script> consectetur adipiscing elit.

I think this feature is absolutely safe, because if you have empty <script> body you can safely replace repeating whitespaces to single whitespace.

Real case in real website:

Snímek obrazovky 2019-08-31 v 17 56 23

Thanks.

{
if ($phase & PHP_OUTPUT_HANDLER_START) {
$s = ltrim($s);
}
if ($phase & PHP_OUTPUT_HANDLER_FINAL) {
$s = rtrim($s);
}
return preg_replace_callback(
$return = (string) preg_replace_callback( // Other cases
Copy link
Member

Choose a reason for hiding this comment

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

Why (string)?

Copy link
Author

Choose a reason for hiding this comment

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

Because PhpStorm hint to me new type can be returned in future. For example null in case of error.

'#[ \t\r\n]+|<(/)?(textarea|pre|script)(?=\W)#si',
function ($m) use (&$strip) {
static function (array $m) use (&$strip): string {
Copy link
Member

Choose a reason for hiding this comment

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

Why static?

Copy link
Author

Choose a reason for hiding this comment

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

PHP does not have to create a new instance every time.

},
$s
);
$return = (string) preg_replace_callback( // <script> for include JS file
'/<script\s*([^>]+?)>(?:\s*)<\/script>/',
Copy link
Member

Choose a reason for hiding this comment

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

Will this regexp work with input like <script title="> <\/script>"> ?

Copy link
Author

Choose a reason for hiding this comment

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

This input is not supported now. I'll try to come up with a better solution.

@dg dg force-pushed the master branch 3 times, most recently from 1a297fd to 8fb996f Compare September 18, 2019 14:37
@dg dg force-pushed the master branch 4 times, most recently from 4d5343b to 6edda93 Compare November 20, 2019 11:58
@dg dg force-pushed the master branch 3 times, most recently from 090695a to dba2a60 Compare December 17, 2019 04:36
@dg dg force-pushed the master branch 10 times, most recently from d3ff96d to 47c1926 Compare December 23, 2019 00:58
@dg dg force-pushed the master branch 2 times, most recently from 942f9b1 to 753a2fc Compare January 3, 2020 22:38
@dg dg force-pushed the master branch 9 times, most recently from 8d92896 to 1267554 Compare May 27, 2024 11:56
@dg dg force-pushed the master branch 4 times, most recently from ee646d6 to b80c3a6 Compare June 18, 2024 21:40
@dg dg force-pushed the master branch 10 times, most recently from 6c48587 to bd46751 Compare August 9, 2024 02:30
@dg dg force-pushed the master branch 7 times, most recently from 6baf2c5 to 4db7a55 Compare October 8, 2024 00:59
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.

2 participants