Refacttored HTML5 class & added HTML5 Helper with static methods #37
Conversation
|
Wow. That was a huge task. Thanks! I just finished reviewing the last two commits, and largely I'm really happy with this solution. Conceptually, I am now sure this is the right way to go. Coding convention-wise, there are a few places where new function calls start with the curly brace on the next line. That's the only one I'd really gripe about. We've also tended to start I'll let @mattfarina review, and if @miso-belica wants to give an opinion, too, I'd like to hear it. I guess now is the time when we need to decide whether we want to do this on a separate branch. |
|
I have fixed some code stye issues. New branch, things to do (in my opinion):
|
|
Can be also a good idea switch also to |
| /** | ||
| * Global options for the parser and serializer. | ||
| * @var array | ||
| */ | ||
| public static $options = array( | ||
|
|
||
| private $options = array( |
There was a problem hiding this comment.
Is there a reason $options is private rather than protected? Isn't protected more appropriate?
There was a problem hiding this comment.
protected equals to "hidden" dependency.
When someone extends HTML5 class, we can't change it's internal implementation.
There was a problem hiding this comment.
Because the visibility is private (where only the class that defines it can access it) methods that use it can't be overridden and still have access to the options (including the constructor). An extending class can't change the defaults.
There was a problem hiding this comment.
I can replace all uses of $this->options with $this->getOptions(). Can be a solution?
You can overwrite getOptions method changing the default values..
|
@goetas Thanks for working on this. I agree with @technosophos that this is a good way to go. It seems sane. @goetas It's common for a projects namespacing to not include the vendor. What is your case for making the switch? I'm not fundamentally opposed to the idea but I'd like to know your case. |
|
Just a "convention" issue. I think that exists somewhere, a developer that has created a class called Adding a namespace prefix, we will become more standard & compatible with the world. |
|
Can it be ready to be merged? |
goetas
changed the title
|
@goetas I'll take a look at this in the next day or so. We had a holiday weekend and then I was under the weather. |
|
@mattfarina don't worry! :) |
|
FYI, since this makes API changes my thought is to get a couple other non-api changing bug fixes in and create another 1.x release. Then make a 1.0 branch. Turn master into a 2.x-dev branch and merge this api changer in (note, i still need to finish my last review). |
|
Just a little question: is HTML5Helper really needed? Since we are breaking compatibility with 1.0, we could avoid this... At the moment there is no real real reason to preserve this class except for compatibility ... |
|
At the moment this class represents 200 lines of code that needs to be mantained, without a real benefit |
|
I don't have a strong opinion about the HTML5Helper class. Personally, I probably wouldn't use it (which is probably a good indicator that it is unnecessary). But it does at least superficially make the new API look like the old one. |
|
I had the exact same thought at @technosophos. While I won't use it, would it be useful to have the new API look like the old one? |
|
Since we are breaking compatibility, new users should learn to use new |
|
Okay. Let's definitely get rid of the statics. |
|
@technosophos 👍 🍰 |
|
+1 to adding the Masterminds namespace. It feels cleaner that way. Oh, and this change will make it much easier to use this in a project I'm working on, where I suspect I'll have to extend the html5 parser to support xml-like namespaces for some template generation (something that the basic html5 lib doesn't need). Thanks! |
| @@ -83,18 +91,17 @@ public static function loadHTML($string) { | |||
| * PHP DOM implementation. It simply calls load(). | |||
| * | |||
| * @param string $file | |||
There was a problem hiding this comment.
You changed the param to $string
Refactored HTML5 class, removed static methods
Refactoring
HTML5class to be as #33 (comment)