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.

Sign up for GitHub

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

Jump to bottom

Automatically convert head[profile] to head > link[rel=profile] #4131

Closed
westonruter opened this issue Jan 18, 2020 · 6 comments · Fixed by #4193
Closed

Automatically convert head[profile] to head > link[rel=profile] #4131

westonruter opened this issue Jan 18, 2020 · 6 comments · Fixed by #4193
Assignees
@kienstra
Labels
Validation
Milestone
v1.5

Comments

@westonruter
Copy link
Member

westonruter commented Jan 18, 2020 •
edited
Loading

Feature description

When doing an analysis of themes on WordPress, I got a DISALLOWED_ATTR validation error for the profile attribute on the head element, such as code in :

<head profile="http://gmpg.org/xfn/11">

The profile attribute on the head element is part of HTML4 but not HTML5.

The W3 validator provides this advice:

image

According to this SO answer, the right way to represent this in HTML5 is:

<head>
    <!-- … -->
    <link rel="profile" href="http://gmpg.org/xfn/11">

This should perhaps be done as part of the normalization done in the Document class (appending the link element to the end of the head). It could also be done in a separate sanitizer, though that may be overkill. Since the Document class is normalizing to HTML5, I think it makes sense to let the class handle the profile transformation.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry
@kienstra
Copy link
Contributor

Discussing Implementation Brief

Hi @westonruter,
Hope you're having a great week.

Assuming it's alright for me to work on this, what do you think about this plan:

  1. Add a new method like Document::convert_head_profile_to_link()
diff --git a/src/Dom/Document.php b/src/Dom/Document.php
index 5bad55ddd..5979e983d 100644
--- a/src/Dom/Document.php
+++ b/src/Dom/Document.php
@@ -350,6 +350,8 @@ final class Document extends DOMDocument {
                                $this->head->removeChild( $meta );
                        }
 
+                       $this->convert_head_profile_to_link();
+
                        // Add the required utf-8 meta charset tag.
                        $charset = $this->createElement( 'meta' );
                        $charset->setAttribute( 'charset', self::AMP_ENCODING );

Placing it in that location will mean that the new <link> will be in the <head> after the <meta charset="utf-8">

  1. As you suggested, create a new <link rel="profile" href="https://foo.com">, and insert it in the <head>
  2. Strip the head[profile] attribute

Thanks, Weston!

@kienstra kienstra self-assigned this Jan 29, 2020
@westonruter
Copy link
Member Author

  1. Add a new method like Document::convert_head_profile_to_link()

Yes, and make it private.

Placing it in that location will mean that the new <link> will be in the <head> after the <meta charset="utf-8">

It can be appended to the head. It doesn't need to be right after the meta tag.

  • As you suggested, create a new <link rel="profile" href="https://foo.com">, and insert it in the <head>

The value of the href will naturally be the value of the profile attribute.

@kienstra
Copy link
Contributor

Thanks, Weston! That sounds good.

@kienstra kienstra mentioned this issue Jan 29, 2020
Merged
3 tasks
@kienstra
Copy link
Contributor

kienstra commented Jan 29, 2020 •
edited
Loading

Hi @westonruter,
Thanks a lot for reviewing and merging #4193. What do you think about moving this to 'Done,' without QA?

I'm not sure what functional testing is possible, other than a review from another developer.

Thanks, Weston!

@westonruter westonruter added this to the v1.5 milestone Jan 29, 2020
@westonruter
Copy link
Member Author

Yeah. Moved to done.

@kienstra
Copy link
Contributor

Thanks, Weston!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kienstra kienstra

Labels
Validation
Projects
None yet
Milestone
v1.5
Development

Successfully merging a pull request may close this issue.

Convert head[profile] attribute to link[rel=profile] ampproject/amp-wp
3 participants
@westonruter @kmyram @kienstra