Add PropertyString, cleanHtml helper and escapeOrCleanHtml helper. #4004

EreMaijala · 2024-10-11T10:20:21Z

Related to #3998, this is a draft for PropertyString that can carry additional information along with a plain text string. It now has explicit support for HTML content, and there's also a cleanHtml helper to handle sanitization. escapeHtmlExt is a replacement for escapeHtml allowing the HTML version to be returned if desired.

@maccabeelevine What do you think? Obviously this needs some template changes where HTML is desirable, but should otherwise be fairly easy to use.

TODO:

Add unit tests

demiankatz

Thanks, @EreMaijala, looks like a great start! A few minor thoughts/suggestions...

themes/bootstrap5/templates/RecordDriver/DefaultRecord/data-summary.phtml

demiankatz · 2024-10-11T13:58:46Z

module/VuFind/src/VuFind/View/Helper/Root/EscapeHtmlExt.php

+ * @license  http://opensource.org/licenses/gpl-2.0.php GNU General Public License
+ * @link     https://vufind.org/wiki/development Wiki
+ */
+class EscapeHtmlExt extends \Laminas\View\Helper\Escaper\AbstractHelper


I don't love this name, but I also can't think of a better one... :-)

Me neither. I wanted to extend EscapeHtml, but the silly @Final thing prevents that. And while we could substitute our own EscapeHtml, the Laminas class is used in so many places, that it's not quite straightforward. We could of course change all the references to an interface, but that would require more wide-ranging changes. If you think that'd be a better way forward, I'd be happy to work on that. What would support that is the fact that EscapeHtmlAttr needs a substitute as well (to be able to control the IE-compatibility of the escaping process), so doing both at the same time would probably make sense.

I don't have really strong feelings on this -- no solution feels obviously like the best. Maybe @maccabeelevine will have some thoughts to share... ;-)

And while we could substitute our own EscapeHtml, the Laminas class is used in so many places, that it's not quite straightforward.

This was certainly my original idea with #3998, to make the template changes as simple as possible, and it's safer here than in my PoC implementation since it would do nothing unless you had a PropertyStringInterface value and passed allowHtml. But I'm sure there are complications I'm not seeing.

A month later, a fresh thought on the name: would escapeOrCleanHtml be more descriptive than escapeHtmlExt, since that's what this is doing -- escaping the HTML unless told to clean it instead?

If Ere agrees, I like that naming!

I like it a lot better than escapeHtmlExt!

module/VuFind/src/VuFind/View/Helper/Root/EscapeHtmlExt.php

module/VuFind/src/VuFind/RecordDriver/DefaultRecord.php

maccabeelevine

I love the idea here, and it's so much better than the original #3998 PoC. I added some specific questions, but I guess my primary question (and maybe the hardest to answer) is how we deal with generic templates that may or may not have HTML in certain fields for some backends.

So, would it be appropriate in the default record driver's core.html to change

<h1<?=$this->schemaOrg()->getAttributes(['property' => 'name'])?>><?=$this->escapeHtml($this->driver->getShortTitle() . ' ' . $this->driver->getSubtitle() . ' ' . $this->driver->getTitleSection())?></h1>

to

<h1<?=$this->schemaOrg()->getAttributes(['property' => 'name'])?>><?=$this->escapeHtmlExt($this->driver->getShortTitle(), allowHtml: true)?> <?=$this->escapeHtml($this->driver->getSubtitle() . ' ' . $this->driver->getTitleSection())?></h1>

Performance-wise, this should be ok, since for any other record driver the title would not be a PropertyString and so the allowHtml would be ignored. But is it ok from a template complexity standpoint?

module/VuFind/src/VuFind/Content/Summaries/Demo.php

module/VuFind/src/VuFind/View/Helper/Root/CleanHtmlFactory.php

maccabeelevine · 2024-10-14T13:49:35Z

module/VuFind/src/VuFind/View/Helper/Root/EscapeHtmlExt.php

+ * @license  http://opensource.org/licenses/gpl-2.0.php GNU General Public License
+ * @link     https://vufind.org/wiki/development Wiki
+ */
+class EscapeHtmlExt extends \Laminas\View\Helper\Escaper\AbstractHelper


And while we could substitute our own EscapeHtml, the Laminas class is used in so many places, that it's not quite straightforward.

This was certainly my original idea with #3998, to make the template changes as simple as possible, and it's safer here than in my PoC implementation since it would do nothing unless you had a PropertyStringInterface value and passed allowHtml. But I'm sure there are complications I'm not seeing.

demiankatz · 2024-10-14T15:33:54Z

Regarding @maccabeelevine's question about changing the default driver's core.html to support HTML, I don't think I would have a problem with that in the interest of greater flexibility....

EreMaijala · 2024-10-15T06:16:56Z

@demiankatz Do we want to support HTML for all fields or for longer textual fields like summary? Different choices will lead to different complications e.g. with search term highlighting.

Co-authored-by: Demian Katz <[email protected]>

demiankatz · 2024-10-15T10:46:35Z

@demiankatz Do we want to support HTML for all fields or for longer textual fields like summary? Different choices will lead to different complications e.g. with search term highlighting.

If we wanted to start small, I would think the bare minimum would be longer textual fields and titles. But I'm not opposed to rolling out the support more widely if it's easier to do it all at once than piecemeal. I tend to favor the most pragmatic strategy, whatever that turns out to be. :-)

EreMaijala · 2024-10-25T13:17:01Z

Here are a couple of thought:

Could add a default "allow HTML" toggle and/or setting to the escaper so that the parameter doesn't need to be repeated. Downside: easy to accidentally leave enabled outside of the intended scope.
Could add configurable defaults per field type ('title', 'alt-title', 'summary' etc.). This would allow specifying the fields with HTML in configuration.
truncate helper is going to have trouble with HTML. Perhaps we can make it smarter, but it won't be easy.
titles will be a problem with any tags that you can't have inside a heading like <h1>. Perhaps we need the field type from 2. to also indicate which tags are allowed.
escapeHtmlExt needs to deviate a bit further from the invoke params available with Laminas' helper. It seems that it'd be better to decouple.

demiankatz · 2024-10-25T13:27:13Z

1. Could add a default "allow HTML" toggle and/or setting to the escaper so that the parameter doesn't need to be repeated. Downside: easy to accidentally leave enabled outside of the intended scope.

I think the downsides probably outweigh the benefits here, since I can't think of any scenario where this doesn't potentially lead to unexpected side effects or confusion. I think it's better to be explicit.

2. Could add configurable defaults per field type ('title', 'alt-title', 'summary' etc.). This would allow specifying the fields with HTML in configuration.

...and this would be a good way to be explicit in a number of contexts.

3. truncate helper is going to have trouble with HTML. Perhaps we can make it smarter, but it won't be easy.

Possible lazy solution: truncate based on a tag-stripped version. If the stripped version is under the limit, display the HTML as-is. If the stripped version is too long, show the truncated, stripped version, and the user will have to "see more" to get the rich version. Not ideal, but maybe a quick place to start.

4. titles will be a problem with any tags that you can't have inside a heading like `<h1>`. Perhaps we need the field type from 2. to also indicate which tags are allowed.

Maybe we need the ability to define multiple named tag allow-lists. Then we could have a "default" list and a "heading" list and the configurable settings could refer to an allow-list, or "none". This would empower users to create their own more granular lists as needed, but we could use these two obvious ones as a starting point.

5. escapeHtmlExt needs to deviate a bit further from the invoke params available with Laminas' helper. It seems that it'd be better to decouple.

Given the way Laminas seems to be gradually cutting off the ability to extend anything, I agree that building tools that meet our needs and wrap around Laminas' public interfaces is better than trying to build upon those interfaces directly.

demiankatz · 2024-11-05T14:33:59Z

This PR was accidentally closed by the deletion of the dev-11.0 branch; I have restored and reopened it. Sorry for the inconvenience!

demiankatz

I took another look at this and had just a few more minor thoughts and questions...

demiankatz · 2024-11-11T19:32:48Z

module/VuFind/src/VuFind/View/Helper/Root/EscapeHtmlExt.php

+ * @license  http://opensource.org/licenses/gpl-2.0.php GNU General Public License
+ * @link     https://vufind.org/wiki/development Wiki
+ */
+class EscapeHtmlExt extends \Laminas\View\Helper\Escaper\AbstractHelper


A month later, a fresh thought on the name: would escapeOrCleanHtml be more descriptive than escapeHtmlExt, since that's what this is doing -- escaping the HTML unless told to clean it instead?

demiankatz · 2024-11-11T19:36:35Z

themes/bootstrap3/templates/RecordDriver/DefaultRecord/data-summary.phtml

+        '<br>',
+        array_map(
+            function ($summary) {
+                $htmlContent = str_starts_with($summary, '<') && str_ends_with($summary, '>');


I wonder if there's a way to eliminate this hacky check for angle brackets. Should we do the check in the record driver and wrap the string in a PropertyString? Should the new helper have a switch to enable this check, or even make this behavior part of the standard "allowHtml"? I'm not totally sure, but it feels like we can do better here, especially since as written, there's no safety on the summary in the special case.

These actually come from the Summaries content loaders. I've added PropertyString support for the Demo loader (module/VuFind/src/VuFind/Content/Summaries/Demo.php), but the Syndetics loader is a bit complicated. I can move the check there, but reading the code it seems like with Syndetics plus it should just output a <div> with the proper id without subjecting it to cleaning or anything (i.e. it would break unless <div> is allowed in clean HTML). I agree that the mechanism isn't quite optimal, though. Perhaps one option would be adding a flag property to PropertyString indicating that the HTML must not be cleaned. Then we could always pass the result to escapeOrCleanHtml() and it would just output the original HTML when the flag is set. What do you think?

Yes, I think adding a flag like "trusted HTML" is probably the best solution. This could be potentially useful in other places where we know HTML has come from a safe source but may have complex characteristics.

Done, but I can't actually test it because I don't have access to SyndeticsPlus.

module/VuFind/src/VuFind/View/Helper/Root/CleanHtmlFactory.php

EreMaijala · 2025-01-03T13:06:26Z

@demiankatz @maccabeelevine Okay, this should finally be up to date wrt the discussion. What do you think?

maccabeelevine

Do I remember correctly that the use case once this is merged would be to edit a specific record driver to return a PropertyString instead of generic string from whatever specific methods we want to support HTML values? If so, I think this looks good, and my work on #3991 can be the test case where we refine the approach. Just one suggestion.

maccabeelevine · 2025-01-03T13:58:02Z

module/VuFind/src/VuFind/String/PropertyString.php

+     *
+     * @return static
+     */
+    public function setIds(array $ids): static


I think this is meant for linked data identifiers (i.e. URIs) for the string, but not sure. Is it practical to include some example use case with the demo driver, or should that wait for future PRs?

Which Demo driver did you have in mind? I don't think linked data IDs make a lot of sense in the context of something like book summaries, so I'm not sure how we would demonstrate them there. A better use case would be to add support in the appropriate MARC-based record driver traits, but that feels like a big enough change that it would be better as a follow-up than as part of this PR. I'm open to other ideas, though, if I'm misunderstanding your proposal!

No I agree, better to wait for another PR.

demiankatz

Thanks, @EreMaijala -- I gave this another look and have a few more comments.

I'm not sure if I still have demo access to Syndetics or not; I can try after this round of review is addressed. If I don't, we can see if anyone in the community is currently using it. Might be good to have a wiki page of people willing to help with testing of third-party services for situations like this.

demiankatz · 2025-01-06T13:15:01Z

themes/bootstrap5/templates/RecordDriver/DefaultRecord/data-summary.phtml

+        '<br>',
+        array_map(
+            function ($summary) {
+                return $this->escapeOrCleanHtml($summary, allowHtml: true);


Does this need to have the summary context added to match core.phtml?

demiankatz · 2025-01-06T13:25:09Z

config/vufind/config.ini

+
+; By default HTML is not allowed in record fields, but support for sanitized HTML
+; can be enabled per field type.
+[HTML_Fields]


I think we might be able to make this a little more clear. A few suggestions:

1.) I wonder if the top comment could be something more like "Data retrieved from records is escaped by default, on the assumption that it does not contain HTML. This section can be used to allow limited HTML in specific contexts; set values to true to allow HTML, and leave false to escape content."

2.) I found the "HTML_Fields" section name a bit confusing. Maybe something like Allowed_HTML_Contexts would be better (not that I love that either). I just wonder if "field" might be too limiting, so I rather prefer "context" -- and since all the settings are about turning something on, having a verb seems potentially helpful.

3.) Should we put comments above all the switches clarifying what the contexts really mean?

demiankatz · 2025-01-06T13:28:54Z

themes/bootstrap5/templates/RecordDriver/DefaultRecord/core.phtml

-    <?php if ($summary = isset($summary[0]) ? $this->escapeHtml($summary[0]) : false): ?>
-      <p><?=$this->truncate($summary, 300)?></p>
+    <?php if ($summary = $summary[0] ?? null): ?>
+      <?php $shortSummary = $this->truncate($summary, 300); ?>


Do we need to worry about truncate splitting HTML and causing broken markup? I thought we addressed this somewhere/somehow, but in my quick review just now I don't see where.

demiankatz · 2025-01-06T13:31:09Z

module/VuFind/src/VuFind/String/PropertyStringInterface.php

+     *
+     * @return ?bool
+     */
+    public function getHtmlTrusted(): ?bool;


A nitpick which you're welcome to ignore, but would isHtmlTrusted be a better name for a boolean method? Or do you think it's more important to have parallel get/set methods?

demiankatz · 2025-01-06T13:32:09Z

module/VuFind/src/VuFind/String/PropertyStringInterface.php

+     *
+     * @return static
+     */
+    public function setIds(array $ids): static;


Would it make sense to also have an addId() method, in case we're not able to set all the IDs at once?

demiankatz · 2025-01-06T13:36:20Z

module/VuFind/src/VuFind/View/Helper/Root/CleanHtmlFactory.php

+            // Limit elements allowed in headings
+            // (see https://developer.mozilla.org/en-US/docs/Web/HTML/Content_categories#phrasing_content for more
+            // information):
+            $config->set('HTML.AllowedElements', 'a,b,br,em,i,span');


Should we make this configurable? If not, should we at least reference this factory in config.ini so people know what "sanitized HTML" actually means?

Add PropertyString, cleanHtml helper and escapeHtmlExt helper.

3b17e2a

EreMaijala requested review from demiankatz and maccabeelevine October 11, 2024 10:20

EreMaijala marked this pull request as draft October 11, 2024 10:20

Add EscapeHtmlExt.

be71629

demiankatz requested changes Oct 11, 2024

View reviewed changes

Fixes.

d5c0ca5

maccabeelevine reviewed Oct 14, 2024

View reviewed changes

EreMaijala and others added 5 commits October 15, 2024 11:07

Add a static factory method.

c1ff4e9

Don't try to be too clever.

da9165f

Fix comment.

61ef47e

Fix indentation.

9bcfe47

Co-authored-by: Demian Katz <[email protected]>

Fix indentation more.

1461a00

demiankatz deleted the branch vufind-org:dev November 1, 2024 18:01

demiankatz closed this Nov 1, 2024

demiankatz reopened this Nov 5, 2024

demiankatz changed the base branch from dev-11.0 to dev November 5, 2024 14:33

demiankatz added this to the 11.0 milestone Nov 5, 2024

maccabeelevine mentioned this pull request Nov 6, 2024

Add backend: ProQuest Federated Search Gateway #3991

Open

3 tasks

Merge remote-tracking branch 'origin/dev' into dev-11.0-propertystring

a1210ce

demiankatz requested changes Nov 11, 2024

View reviewed changes

EreMaijala added 3 commits January 3, 2025 13:58

Rename helper, fix truncate, add configuration for HTML fields.

efa191a

Merge remote-tracking branch 'origin/dev' into dev-11.0-propertystring

bb45b75

Fix code style.

5975e48

EreMaijala added 2 commits January 3, 2025 15:02

Improve summaries.

9f520cf

Add context support for cleaning HTML.

3d5df26

maccabeelevine approved these changes Jan 3, 2025

View reviewed changes

demiankatz requested changes Jan 6, 2025

View reviewed changes

EreMaijala changed the title ~~Add PropertyString, cleanHtml helper and escapeHtmlExt helper.~~ Add PropertyString, cleanHtml helper and escapeOrCleanHtml helper. Jan 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PropertyString, cleanHtml helper and escapeOrCleanHtml helper. #4004

Add PropertyString, cleanHtml helper and escapeOrCleanHtml helper. #4004

EreMaijala commented Oct 11, 2024

demiankatz left a comment

demiankatz Oct 11, 2024

EreMaijala Oct 11, 2024

demiankatz Oct 11, 2024

maccabeelevine Oct 14, 2024

demiankatz Nov 11, 2024

maccabeelevine Nov 12, 2024

EreMaijala Nov 12, 2024

maccabeelevine left a comment •

edited

Loading

maccabeelevine Oct 14, 2024

demiankatz commented Oct 14, 2024

EreMaijala commented Oct 15, 2024

demiankatz commented Oct 15, 2024

EreMaijala commented Oct 25, 2024

demiankatz commented Oct 25, 2024

demiankatz commented Nov 5, 2024

demiankatz left a comment

demiankatz Nov 11, 2024

demiankatz Nov 11, 2024

EreMaijala Jan 3, 2025

demiankatz Jan 3, 2025

EreMaijala Jan 3, 2025

EreMaijala commented Jan 3, 2025

maccabeelevine left a comment

maccabeelevine Jan 3, 2025

demiankatz Jan 6, 2025

maccabeelevine Jan 6, 2025

demiankatz left a comment

demiankatz Jan 6, 2025

demiankatz Jan 6, 2025

demiankatz Jan 6, 2025

demiankatz Jan 6, 2025

demiankatz Jan 6, 2025

demiankatz Jan 6, 2025

Add PropertyString, cleanHtml helper and escapeOrCleanHtml helper. #4004

Are you sure you want to change the base?

Add PropertyString, cleanHtml helper and escapeOrCleanHtml helper. #4004

Conversation

EreMaijala commented Oct 11, 2024

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maccabeelevine left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz commented Oct 14, 2024

EreMaijala commented Oct 15, 2024

demiankatz commented Oct 15, 2024

EreMaijala commented Oct 25, 2024

demiankatz commented Oct 25, 2024

demiankatz commented Nov 5, 2024

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EreMaijala commented Jan 3, 2025

maccabeelevine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maccabeelevine left a comment •

edited

Loading