Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Incorrect double validation for language tag in ApplyOptionsToTag #52

Closed
gsathya opened this issue Aug 8, 2018 · 3 comments
Closed

Comments

@gsathya
Copy link
Member

gsathya commented Aug 8, 2018

In ApplyOptionsToTag,

4. If language is not undefined, then
       a. If language does not match the language production, throw a RangeError exception.
       b. If language matches the grandfathered production, throw a RangeError exception.

which means that language can not match a grandfathered production after this point.

Later in ApplyOptionsToTag,

 10. If language is not undefined,
        a. If tag matches the privateuse or grandfathered production,
            i. Set tag to language.
           ii. If tag matches the grandfathered production,
                1. Set tag to CanonicalizeLanguageTag(tag).

We've already checked to see if the language tag matches a grandfathered production in step 4. I don't see why we require another validation and canonicalization in steps 10. a. ii and 10. a. ii. 1

littledan added a commit that referenced this issue Aug 8, 2018
This is a follow-up to #50, to remove an error check which is
now subsumed by a later conditional.

Closes #52
littledan added a commit that referenced this issue Aug 8, 2018
This is a follow-up to #50, to remove an error check which is
now subsumed by a later conditional.

Closes #52
@jackhorton
Copy link

jackhorton commented Sep 18, 2018

Also, step 3 says to match the language option against the RFC5646 language production, which does not include grandfathered tags. So 4.b can't be hit anyways?

EDIT 1:
Ah I see the issue here. the commit fixes the comment I just made, but the GitHub.io rendered version doesn't reflect the update.

Could we get a new rendering?

EDIT 2:
The commit is still in PR. I obviously need to go take a nap. Anyways, I await the resolution of this soon :)

@littledan
Copy link
Member

The PR at #57 could help some, but I'm not landing it yet because of the concerns @anba raised; anyway, this might all be a lot simpler once we resolve #63

@littledan
Copy link
Member

Fixed by #66 .

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants