use shared laminas escaper instance

[pine3ree]

Describe the PR

Use a single laminas escaper instance instead of a different instance per method call. This is possible as all instances are created with the same utf-8 $encoding and the fact that the escaper internal encoding cannot be changed after creation.

• Fixes: nothing

[bastianallgeier]

@lukasbestle looks good to me, what do you think? But I would probably introduce a new static::escaper() method to clean it up a bit.

[lukasbestle]

@pine3ree Thanks for your PR! I saw your post in the forum and wanted to make the same changes.

I agree with Bastian. To make the code DRY, there should be a new protected static method escaper() that returns the instance or creates a new one if null.

Please also use static:: instead of self:: to allow extending the class. We use static:: throughout the codebase, so this is important for consistency.

[afbora]

@pine3ree If you don't have time, do you want us to take care of minor improvements?

[distantnative]

@pine3ree @afbora I went ahead and implemented as discussed :)

[pine3ree]

Hello @lukasbestle, @bastianallgeier , @afbora , sorry guys...bad health these last couple of days... I though about a ::getEscaper() singleton, but in the last years I'have tried to avoid too many recursive calls if I have shorthand alternatives such as ?? . But of course a static::getEscaper() looks prettier.

I would avoid (but it doesn't hurt) adding escapeHtmlAttr, This method is meant to be used in pure php-templates to prevent malicious code when html attributes are not being quoted (see https://docs.laminas.dev/laminas-escaper/escaping-html-attributes/#example-of-good-html-attribute-escaping line 22 of the 3rd example). I believe it's better to always properly use (double) quotes for html-attributes in php-templates and utilize the attribute-value standard html-entity-escaper, just to avoid picking up bad habits. :-). What I would rather do is to use the \ENT_QUOTES contanst instead of ENT_COMPAT in Html::encode()...I personally always use double quotes for html-attributes, but single quotes are acceptable and I have seen a few around.

For the other changes let me know If you prefer that I add them (and which ones) to by pr or if you pick them from @afbora .

I am just browsing kirby code these days (and enjoying doing it), even if I remember tooking an interest a few years back in the first versions, so I apologise If I say something obvious or obviously wrong.

I noticed that this class is used directly by the esc helper function and by indirectly (via esc function call) by the escape() virtual method. So extending this class will not have effect on those 2 functions which are importing the original class. So maybe some helper classes could also be declared as final. But that said I am not sure if there is a kirby way to substitute the helper classes (or any other like in Silverstripe for instance).

I would also recommend to start importing gobal php functions and constants and not only classes. It's not required but it can speed up php code execution up to 4~5%. kirby is already very fast, but faster is better when not affecting code readability. For this we just need a few more use function ....

kind regards

[lukasbestle]

Thanks for your reply @pine3ree! Don't worry about the code, Nico has already made the changes.

in the last years I'have tried to avoid too many recursive calls if I have shorthand alternatives such as ?? . But of course a static::getEscaper() looks prettier.

Not only that – it makes the code DRY and makes sure that we don't forget changes in one method in case we ever need to change the returned object.

I believe it's better to always properly use (double) quotes for html-attributes in php-templates and utilize the attribute-value standard html-entity-escaper, just to avoid picking up bad habits. :-)

Absolutely! But you never know in which use-cases escaping is needed in a Kirby site. Especially because templates are created by our users and not by us. :)

What I would rather do is to use the \ENT_QUOTES contanst instead of ENT_COMPAT in Html::encode()

You are right, I've opened an issue here: #3291

So maybe some helper classes could also be declared as final. But that said I am not sure if there is a kirby way to substitute the helper classes (or any other like in Silverstripe for instance).

It is currently not possible to substitute the classes that are being used by the helpers. But we don't want to make our classes final anyway as that would hinder users from extending them for their own purposes. Our philosophy is to provide as much flexibility as possible and such strict class definitions don't fit to that.

I would also recommend to start importing gobal php functions and constants and not only classes. It's not required but it can speed up php code execution up to 4~5%. kirby is already very fast, but faster is better when not affecting code readability. For this we just need a few more use function ....

Oh interesting, I never thought of that. There's one minor issue: We currently abuse this PHP feature to mock PHP functions in tests. For example we override the time() function with a mock so that we can control its return value. Do you have an idea how to solve this elegantly when mocking the function is no longer possible?

[pine3ree]

@lukasbestle, :-) That's (the?) one good thing about global functions lookup...intercepting and replacing them for unit testing. As I said, It will take time to look at every aspects of kirby. So, apologies for having made you spend time to explain.

About the class substitution, don't get me wrong. I am not a fan. I was just having doubts about the existence of some - unknown to me - kirby mechanism to allow subclassing static helper classes and have them being automatically used by any framework/field method relying on their original ancestors.

kind regards.

[lukasbestle]

So, apologies for having made you spend time to explain.

Don't worry, it's great to get feedback like yours. Thank you! 👍