-
Notifications
You must be signed in to change notification settings - Fork 113
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
Add more extensions on composer.json, improve phpdocs and remove dead code #145
Conversation
@@ -136,6 +136,7 @@ class DOMTreeBuilder implements EventHandler | |||
protected $stack = array(); | |||
|
|||
protected $current; // Pointer in the tag hierarchy. | |||
protected $rules; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property was dynamically set in the constructor.
@@ -126,7 +131,7 @@ public function currentLine() | |||
*/ | |||
public function getCurrentLine() | |||
{ | |||
return currentLine(); | |||
return $this->currentLine(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was a bug.
579445c
to
8bcaeec
Compare
composer.json
Outdated
"ext-dom": "*", | ||
"ext-iconv": "*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iconv is used only as fallback when mbstring is not present, so technically both iconv and mbstring are optional (but at least one of them is required)
Any idea on how to express this in composer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I don't think there is any way to ask Composer for that. Let's keep an exception :) .
8bcaeec
to
80b8e91
Compare
Updated :) |
Thanks! |
No problem, I didn't update it before anyway :) . Thanks for the merge! |
This PR proposes a few things:
Don't hesitate to answer my comments when you think I'm wrong :) !