RFC: Should Includes helpers returning Html return HtmlStrings?

[caendesilva]

We have this class, where we document that "Both Markdown and Blade includes will be rendered to HTML". Wouldn't it then make more sense if these returned HtmlString objects instead of string types?

/**
 * The Includes facade provides a simple way to access partials in the includes directory.
 *
 * Both Markdown and Blade includes will be rendered to HTML.
 */
class Includes

This is more semantic, but has the side effect that they will no longer be escaped.

Considering the usages though, I imagine that these are already being used within literal echo directives {!! !!}. It could be a breaking change, and it needs to be communicated clearly, so possibly something for v2.x?

[caendesilva]

GPT-4o Response

Feedback on RFC: Should Includes Helpers Returning Html Return HtmlStrings?

Summary

The proposal suggests modifying the return type of HTML content from plain strings to HtmlString objects in the Includes class of the HydePHP static site generator. This change aims to improve semantic correctness by explicitly indicating that the returned content is HTML and should not be escaped.

Points in Favor

1. Semantic Clarity:

   • Returning HtmlString instead of plain strings makes the intent explicit. It clearly indicates that the content is HTML and should be handled appropriately by the rendering system.

2. Preventing Double Escaping:

   • By returning HtmlString, you avoid the risk of double escaping HTML content. This ensures that the content is rendered correctly when included in views.

3. Alignment with Documentation:

   • The current documentation states that "Both Markdown and Blade includes will be rendered to HTML". Using HtmlString aligns the implementation with this documentation, providing consistency and clarity.

Points of Concern

1. Breaking Changes:

   • Changing the return type to HtmlString can break existing implementations that expect a plain string. This requires clear communication and possibly a major version update (e.g., v2.x).

2. Security Implications:

   • HtmlStrings are not escaped by default, which can introduce security risks if the content is not properly sanitized. This change requires careful consideration of how content is sourced and processed to avoid XSS vulnerabilities.

3. Usage Considerations:

   • The current implementation might be relying on {!! !!} directives to prevent escaping. While changing to HtmlString could simplify usage, it might require developers to review and update existing code to ensure compatibility.

Recommendations

1. Versioning and Communication:

   • Implement this change in a major version update (v2.x) to minimize disruption. Clearly document the change and provide migration guidelines for users.

2. Sanitization and Security:

   • Ensure that all HTML content returned as HtmlString is properly sanitized before rendering. This might involve introducing additional sanitization steps in the Includes class.

3. Backward Compatibility:

   • Consider providing a configuration option to toggle between plain strings and HtmlString during a transition period. This allows users to gradually adapt to the new behavior.

4. Comprehensive Testing:

   • Thoroughly test the change with various scenarios to ensure that it doesn't introduce unexpected issues. Pay special attention to security and escaping behavior.

Proposed Changes to the Class

Here’s an example of how the Includes class methods can be updated to return HtmlString objects:

use Illuminate\Support\HtmlString;

class Includes
{
    // ... other methods remain unchanged

    /**
     * Get the HTML contents of a partial file in the includes directory.
     *
     * @param  string  $filename  The name of the partial file, with or without the extension.
     * @param  string|null  $default  The default value to return if the partial is not found.
     * @return HtmlString|null The raw contents of the partial file, or the default value if not found.
     */
    public static function html(string $filename, ?string $default = null): ?HtmlString
    {
        $path = static::path(basename($filename, '.html').'.html');

        if (! file_exists($path)) {
            return $default === null ? null : new HtmlString($default);
        }

        return new HtmlString(file_get_contents($path));
    }

    /**
     * Get the rendered Markdown of a partial file in the includes directory.
     *
     * @param  string  $filename  The name of the partial file, with or without the extension.
     * @param  string|null  $default  The default value to return if the partial is not found.
     * @return HtmlString|null The rendered contents of the partial file, or the default value if not found.
     */
    public static function markdown(string $filename, ?string $default = null): ?HtmlString
    {
        $path = static::path(basename($filename, '.md').'.md');

        if (! file_exists($path)) {
            return $default === null ? null : new HtmlString(Markdown::render($default));
        }

        return new HtmlString(Markdown::render(file_get_contents($path)));
    }

    /**
     * Get the rendered Blade of a partial file in the includes directory.
     *
     * @param  string  $filename  The name of the partial file, with or without the extension.
     * @param  string|null  $default  The default value to return if the partial is not found.
     * @return HtmlString|null The rendered contents of the partial file, or the default value if not found.
     */
    public static function blade(string $filename, ?string $default = null): ?HtmlString
    {
        $path = static::path(basename($filename, '.blade.php').'.blade.php');

        if (! file_exists($path)) {
            return $default === null ? null : new HtmlString(Blade::render($default));
        }

        return new HtmlString(Blade::render(file_get_contents($path)));
    }
}

This change ensures that the methods return HtmlString objects, indicating that the content is HTML and should not be escaped, while also aligning with the semantic intent described in the documentation.

[caendesilva]

If we move forward, the upgrade path should have instructions for how current usages can be simplified ({!! => {{) and how to escape includes if that is needed (adding e())

[caendesilva]

Considering that the use case for this is often to include HTML partials (literal, Markdown, or Blade), I think it makes sense for this to be HtmlStrings so they can be rendered without having to use !! syntax. Also consider that using @include in Blade doesn't escape the partial.

[caendesilva]

Implemented in #1738