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.

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

Fixed alphabet, ß now can be detected #73

Merged
merged 1 commit into from
Jan 20, 2015

Conversation

Westie
Copy link
Contributor

@Westie Westie commented Jan 15, 2015

The english alphabet does not allow the substitution of a lowercase z in place of another uppercase A.

The english alphabet does not allow the substitution of a lowercase z in place of another uppercase A.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5466154 on Westie:master into 398ebb6 on Masterminds:master.

@goetas
Copy link
Member

goetas commented Jan 15, 2015

hi! can you create a test case for this? I'm still not able to understand the PR.... :-(

@Westie
Copy link
Contributor Author

Westie commented Jan 15, 2015

Okay, what's the best way to attach a test case? Stick it in here, or close this and start again?

@Westie
Copy link
Contributor Author

Westie commented Jan 15, 2015

Anyway, I'm at work at the moment so I'll have to make a test case later. Anyway, what the issue was is that before, the ALPHA constants were like this:

abcdefAghijklmnopqrstuvwxy

Now, you'll notice that there is a stray 'A' in there, and the 'z' is missing. Therefore, ß would be detected as &s, and z would cause another error because ; was expected.

Changing it to:

abcdefghijklmnopqrstuvwxyz

...fixes the issue. Hopefully that should explain it enough for you. :)

@goetas
Copy link
Member

goetas commented Jan 15, 2015

You can add a second commit, pushing again this branch.

You can add a test here Masterminds\HTML5\Tests\Parser\DOMTreeBuilderTest or Masterminds\HTML5\Tests\Parser\TokenizerTest (it the depends on the propose of your PR).

@Westie
Copy link
Contributor Author

Westie commented Jan 15, 2015

Yeah, I don't think this really worth a test case. Plus, what would it be testing? A class constant?

@goetas
Copy link
Member

goetas commented Jan 15, 2015

@technosophos Do you know why we have this "A" in this constant "abcdefAghijklmnopqrstuvwxyABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890" ?

It is a part of a very old commit. May be just a typo?

@goetas
Copy link
Member

goetas commented Jan 15, 2015

@Westie I will investigate on that, but most probably it is just a typo....

@technosophos
Copy link
Member

The extra A is a typo.

@goetas
Copy link
Member

goetas commented Jan 20, 2015

Good!

goetas added a commit that referenced this pull request Jan 20, 2015
Fixed a typo on HTML5 Scanner class
@goetas goetas merged commit 700717e into Masterminds:master Jan 20, 2015
@mattfarina
Copy link
Member

This error would still exist on the 1.x branch. Do we know if people are still using that branch? Maybe cherry-pick this bug fix over there and release a bugfix release. Any thoughts?

@goetas
Copy link
Member

goetas commented Jan 21, 2015

@mattfarina Unfortunately https://github.com/composer/packagist does not provide the number of downloads for each version (or some kind of aggregate grouped value per major release). / cc @Seldaek

But a 1.x bugfix release can be useful...

@mattfarina
Copy link
Member

Released 1.0.6 with this fix in it.

@goetas
Copy link
Member

goetas commented Jan 21, 2015

👍

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

Successfully merging this pull request may close these issues.

None yet

5 participants