Hi, I've seen recently a lot of code like this:
$html = Html::openElement( 'div', array( 'class' => 'foo' ) . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . Html::closeElement( 'div' );
IMO, cruft like this makes things harder to read and adds additional performance overhead. It can be simplified to
$html = '<div class="foo'><p>' . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . '</p></div>';
What's your opinion, guys and gals?
I think your question answers itself, because you have a syntax error in your suggested HTML string:
$html = '<div class="foo'><p>' ...
Use the Html class and you'll have fewer worries about malformed HTML.
WordPress code is full of hard-coded HTML strings and it's maddening to read and understand. Personally I'm thankful for MediaWiki's Html class to keep things orderly.
DanB
-----Original Message----- From: wikitech-l-bounces@lists.wikimedia.org [mailto:wikitech-l-bounces@lists.wikimedia.org] On Behalf Of Max Semenik Sent: Monday, May 13, 2013 1:27 PM To: Wikimedia developers Subject: [Wikitech-l] Code style: overuse of Html::element()
Hi, I've seen recently a lot of code like this:
$html = Html::openElement( 'div', array( 'class' => 'foo' ) . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . Html::closeElement( 'div' );
IMO, cruft like this makes things harder to read and adds additional performance overhead. It can be simplified to
$html = '<div class="foo'><p>' . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . '</p></div>';
What's your opinion, guys and gals?
-- Best regards, Max Semenik ([[User:MaxSem]])
_______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Mon, May 13, 2013 at 10:26 AM, Max Semenik maxsem.wiki@gmail.com wrote:
Hi, I've seen recently a lot of code like this:
$html = Html::openElement( 'div', array( 'class' => 'foo' ) . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . Html::closeElement( 'div' );
IMO, cruft like this makes things harder to read and adds additional performance overhead. It can be simplified to
$html = '<div class="foo'><p>' . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . '</p></div>';
What's your opinion, guys and gals?
I'm probably a bad offender here, but you've unintentionally proved my point ;). Note that in your example, you used a single instead of a double quote after foo. Obviously, if you're using an IDE, syntax highlighting would have helped you, but my point being that when you use the classes, you're less likely to make those little mistakes that could potentially have disastrous consequences (like using single quotes around an entity and relying on htmlspecialchars for escaping, etc). And for security, I prefer for people to use whatever will cause the least amount of mistakes.
Personally also, when I'm code reviewing I don't like to see <> in the php, but that's my person preference.
Chris makes a good point. Also, it should be noted that the Html class does a lot more than just escape stuff. It does a whole bunch of attribute validation and standardization to make output HTML5-sanitary. While in simple cases like the one above it will not make a difference, it is probably better to maintain a uniform approach when generating HTML output.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Mon, May 13, 2013 at 2:05 PM, Chris Steipp csteipp@wikimedia.org wrote:
On Mon, May 13, 2013 at 10:26 AM, Max Semenik maxsem.wiki@gmail.com wrote:
Hi, I've seen recently a lot of code like this:
$html = Html::openElement( 'div', array( 'class' => 'foo' ) . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . Html::closeElement( 'div' );
IMO, cruft like this makes things harder to read and adds additional performance overhead. It can be simplified to
$html = '<div class="foo'><p>' . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . '</p></div>';
What's your opinion, guys and gals?
I'm probably a bad offender here, but you've unintentionally proved my point ;). Note that in your example, you used a single instead of a double quote after foo. Obviously, if you're using an IDE, syntax highlighting would have helped you, but my point being that when you use the classes, you're less likely to make those little mistakes that could potentially have disastrous consequences (like using single quotes around an entity and relying on htmlspecialchars for escaping, etc). And for security, I prefer for people to use whatever will cause the least amount of mistakes.
Personally also, when I'm code reviewing I don't like to see <> in the php, but that's my person preference.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Also, standards can change sometimes and same tags may change. It's better to change a thing in one place than chasing the mysterious bug.
On Mon, May 13, 2013 at 9:22 PM, Tyler Romeo tylerromeo@gmail.com wrote:
Chris makes a good point. Also, it should be noted that the Html class does a lot more than just escape stuff. It does a whole bunch of attribute validation and standardization to make output HTML5-sanitary. While in simple cases like the one above it will not make a difference, it is probably better to maintain a uniform approach when generating HTML output.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Mon, May 13, 2013 at 2:05 PM, Chris Steipp csteipp@wikimedia.org wrote:
On Mon, May 13, 2013 at 10:26 AM, Max Semenik maxsem.wiki@gmail.com wrote:
Hi, I've seen recently a lot of code like this:
$html = Html::openElement( 'div', array( 'class' => 'foo' ) . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId
),
$somePotentiallyUnsafeText ) ) . Html::closeElement( 'div' );IMO, cruft like this makes things harder to read and adds additional performance overhead. It can be simplified to
$html = '<div class="foo'><p>' . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId
),
$somePotentiallyUnsafeText ) ) . '</p></div>';What's your opinion, guys and gals?
I'm probably a bad offender here, but you've unintentionally proved my point ;). Note that in your example, you used a single instead of a double quote after foo. Obviously, if you're using an IDE, syntax highlighting would have helped you, but my point being that when you use the classes, you're less likely to make those little mistakes that could potentially have disastrous consequences (like using single quotes around an entity and relying on htmlspecialchars for escaping, etc). And for security, I prefer for people to use whatever will cause the least amount of mistakes.
Personally also, when I'm code reviewing I don't like to see <> in the php, but that's my person preference.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Le 13/05/13 19:26, Max Semenik a écrit :
Hi, I've seen recently a lot of code like this:
$html = Html::openElement( 'div', array( 'class' => 'foo' ) . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . Html::closeElement( 'div' );
IMO, cruft like this makes things harder to read and adds additional performance overhead.
Html is just like our Xml class, that let us raise the probability that the result code will be valid. That is also a good way to make sure the content is properly escaped, though in the example above that could lead to some mistake due to all the nested calls.
For the performance overhead, it surely exist but it is most probably negligible unless the methods are in a heavily used code path.
Ideally we would use templates to generate all of that. That will let us extract the views logic out of the PHP code.
On one hand, I prefer to have a properly formatted code, but on the other, most systems i have worked with have a very high cost of string concatenation, and I have a strong suspicion PHP is guilty of that too. Constructing HTML one element/value at a time might prove to be on of the bigger perf bottlenecks.
From my personal experience, once I worked on a networking lib for
proprietary protocol, and noticed that there was a lot of logging calls in the form of Log("value1=" + value1 + " value2=" + value2 ...). After I switched it to the form "Log("value1={0}, value2={1}", value1, value2), the code became an order of magnitude faster because the logging framework deferred concatenation until the last moment after it knew that logging is needed, and the actual concatenation was done for the whole complex string with values, not one substring at a time.
On Mon, May 13, 2013 at 6:10 PM, Antoine Musso hashar+wmf@free.fr wrote:
Le 13/05/13 19:26, Max Semenik a écrit :
Hi, I've seen recently a lot of code like this:
$html = Html::openElement( 'div', array( 'class' => 'foo' ) . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . Html::closeElement( 'div' );
IMO, cruft like this makes things harder to read and adds additional performance overhead.
Html is just like our Xml class, that let us raise the probability that the result code will be valid. That is also a good way to make sure the content is properly escaped, though in the example above that could lead to some mistake due to all the nested calls.
For the performance overhead, it surely exist but it is most probably negligible unless the methods are in a heavily used code path.
Ideally we would use templates to generate all of that. That will let us extract the views logic out of the PHP code.
-- Antoine "hashar" Musso
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Personally as a frontend developer I find maintaining html the Html::openElement way in PHP a nightmare.
Following on from Antoine's post, I experimented recently with using a template engine Mustache that works on both javascript and PHP and allows separation of HTML templates from PHP code. In the mobile team we are increasingly finding overlap in things we need to render in javascript and in PHP. MobileFrontend currently uses Hogan (which is essentially Mustache) in our javascript code base and it helps make our javascript easier to maintain and read. We'd love the same for PHP - there's even a bug for that [1].
These templates make escaping simple - you either do {{variable}} to render escaped or {{{html}}} to render HTML.
My personal belief is taking this approach would lead to much more readable code (especially when it comes to skins). The proof is in the pudding - [2][3]
We also have an HTML validation script [4] that I wrote about earlier which allows us to validate pages and avoid submitting invalid code so I wouldn't use this as an argument against...
[1] https://bugzilla.wikimedia.org/show_bug.cgi?id=44130 [2] https://github.com/jdlrobson/Minerva/blob/evenmorevanilla/Minerva.php#L72 [3] https://github.com/jdlrobson/Minerva/blob/evenmorevanilla/minerva/templates/... [4] http://www.gossamer-threads.com/lists/wiki/wikitech/355021
On Mon, May 13, 2013 at 3:27 PM, Yuri Astrakhan yastrakhan@wikimedia.org wrote:
On one hand, I prefer to have a properly formatted code, but on the other, most systems i have worked with have a very high cost of string concatenation, and I have a strong suspicion PHP is guilty of that too. Constructing HTML one element/value at a time might prove to be on of the bigger perf bottlenecks.
From my personal experience, once I worked on a networking lib for proprietary protocol, and noticed that there was a lot of logging calls in the form of Log("value1=" + value1 + " value2=" + value2 ...). After I switched it to the form "Log("value1={0}, value2={1}", value1, value2), the code became an order of magnitude faster because the logging framework deferred concatenation until the last moment after it knew that logging is needed, and the actual concatenation was done for the whole complex string with values, not one substring at a time.
On Mon, May 13, 2013 at 6:10 PM, Antoine Musso hashar+wmf@free.fr wrote:
Le 13/05/13 19:26, Max Semenik a écrit :
Hi, I've seen recently a lot of code like this:
$html = Html::openElement( 'div', array( 'class' => 'foo' ) . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . Html::closeElement( 'div' );
IMO, cruft like this makes things harder to read and adds additional performance overhead.
Html is just like our Xml class, that let us raise the probability that the result code will be valid. That is also a good way to make sure the content is properly escaped, though in the example above that could lead to some mistake due to all the nested calls.
For the performance overhead, it surely exist but it is most probably negligible unless the methods are in a heavily used code path.
Ideally we would use templates to generate all of that. That will let us extract the views logic out of the PHP code.
-- Antoine "hashar" Musso
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
In the mobile team we are increasingly finding overlap in things we need to render in javascript and in PHP
Out of curiosity -- what are you rendering in JS that you wouldn't already have in the DOM or wouldn't be able to render server side?
We'd love the same for PHP - there's even a bug for that.
Not that it's ready for General release yet but Fundraising has been using Twig for a while in parts of it's code (our thank you email generator, and unsubscribe processor). I enjoy the ability the create code reusable and code-review-able extensions like [1] -- which seems to be a plus for Twig over Mustache. I also enjoy the built in file system loader and template inheritance mechanisms that twig offers.
Chris S. has it in his review queue to security review twig so that I can make my Twig/MediaWiki integration more official.
[1] https://gerrit.wikimedia.org/r/#/c/63252/
~Matt Walker Wikimedia Foundation Fundraising Technology Team
On Mon, May 13, 2013 at 5:23 PM, Jon Robson jdlrobson@gmail.com wrote:
Personally as a frontend developer I find maintaining html the Html::openElement way in PHP a nightmare.
Following on from Antoine's post, I experimented recently with using a template engine Mustache that works on both javascript and PHP and allows separation of HTML templates from PHP code. In the mobile team we are increasingly finding overlap in things we need to render in javascript and in PHP. MobileFrontend currently uses Hogan (which is essentially Mustache) in our javascript code base and it helps make our javascript easier to maintain and read. We'd love the same for PHP
- there's even a bug for that [1].
These templates make escaping simple - you either do {{variable}} to render escaped or {{{html}}} to render HTML.
My personal belief is taking this approach would lead to much more readable code (especially when it comes to skins). The proof is in the pudding - [2][3]
We also have an HTML validation script [4] that I wrote about earlier which allows us to validate pages and avoid submitting invalid code so I wouldn't use this as an argument against...
[1] https://bugzilla.wikimedia.org/show_bug.cgi?id=44130 [2] https://github.com/jdlrobson/Minerva/blob/evenmorevanilla/Minerva.php#L72 [3] https://github.com/jdlrobson/Minerva/blob/evenmorevanilla/minerva/templates/... [4] http://www.gossamer-threads.com/lists/wiki/wikitech/355021
On Mon, May 13, 2013 at 3:27 PM, Yuri Astrakhan yastrakhan@wikimedia.org wrote:
On one hand, I prefer to have a properly formatted code, but on the
other,
most systems i have worked with have a very high cost of string concatenation, and I have a strong suspicion PHP is guilty of that too. Constructing HTML one element/value at a time might prove to be on of the bigger perf bottlenecks.
From my personal experience, once I worked on a networking lib for proprietary protocol, and noticed that there was a lot of logging calls
in
the form of Log("value1=" + value1 + " value2=" + value2 ...). After I switched it to the form "Log("value1={0}, value2={1}", value1, value2),
the
code became an order of magnitude faster because the logging framework deferred concatenation until the last moment after it knew that logging is needed, and the actual concatenation was done for the whole complex string with values, not one substring at a time.
On Mon, May 13, 2013 at 6:10 PM, Antoine Musso hashar+wmf@free.fr
wrote:
Le 13/05/13 19:26, Max Semenik a écrit :
Hi, I've seen recently a lot of code like this:
$html = Html::openElement( 'div', array( 'class' => 'foo' ) . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' =>
$somePotentiallyUnsafeId ),
$somePotentiallyUnsafeText ) ) . Html::closeElement( 'div' );IMO, cruft like this makes things harder to read and adds additional performance overhead.
Html is just like our Xml class, that let us raise the probability that the result code will be valid. That is also a good way to make sure the content is properly escaped, though in the example above that could lead to some mistake due to all the nested calls.
For the performance overhead, it surely exist but it is most probably negligible unless the methods are in a heavily used code path.
Ideally we would use templates to generate all of that. That will let us extract the views logic out of the PHP code.
-- Antoine "hashar" Musso
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
-- Jon Robson http://jonrobson.me.uk @rakugojon
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Currently we are experimenting with lazy loading pages and talk pages that are loaded within a page. Both need templates to render in both PHP and JavaScript. The fact that we use sections in mobile requires looping so it's not possible to determine a template for a page with sections from the html in a stub page for example. We are also thinking about separating language from HTML. The JavaScript would need an equivalent PHP fallback.. (see https://bugzilla.wikimedia.org/show_bug.cgi?id=40678)
twig reminds me of jinja python templates. It looks far too powerful for my liking (casual glance so sorry if I've read this wrong). Personal experience tells me a powerful templating language encourages bad habits. Filters like upper belong in code not templates in my opinion. The thing Juliusz and I liked about Hogan was the fact it was very simple and close to logic less (only has ifs and loops). The dumber the better :) Also is there a JS implementation?
That said I like the idea that MediaWiki could be template agnostic and work with various templating languages and we could RESOLVED LATER a standard :) On 13 May 2013 18:18, "Matthew Walker" mwalker@wikimedia.org wrote:
In the mobile team we are increasingly finding overlap in things we need to render in javascript and in PHP
Out of curiosity -- what are you rendering in JS that you wouldn't already have in the DOM or wouldn't be able to render server side?
We'd love the same for PHP - there's even a bug for that.
Not that it's ready for General release yet but Fundraising has been using Twig for a while in parts of it's code (our thank you email generator, and unsubscribe processor). I enjoy the ability the create code reusable and code-review-able extensions like [1] -- which seems to be a plus for Twig over Mustache. I also enjoy the built in file system loader and template inheritance mechanisms that twig offers.
Chris S. has it in his review queue to security review twig so that I can make my Twig/MediaWiki integration more official.
[1] https://gerrit.wikimedia.org/r/#/c/63252/
~Matt Walker Wikimedia Foundation Fundraising Technology Team
On Mon, May 13, 2013 at 5:23 PM, Jon Robson jdlrobson@gmail.com wrote:
Personally as a frontend developer I find maintaining html the Html::openElement way in PHP a nightmare.
Following on from Antoine's post, I experimented recently with using a template engine Mustache that works on both javascript and PHP and allows separation of HTML templates from PHP code. In the mobile team we are increasingly finding overlap in things we need to render in javascript and in PHP. MobileFrontend currently uses Hogan (which is essentially Mustache) in our javascript code base and it helps make our javascript easier to maintain and read. We'd love the same for PHP
- there's even a bug for that [1].
These templates make escaping simple - you either do {{variable}} to render escaped or {{{html}}} to render HTML.
My personal belief is taking this approach would lead to much more readable code (especially when it comes to skins). The proof is in the pudding - [2][3]
We also have an HTML validation script [4] that I wrote about earlier which allows us to validate pages and avoid submitting invalid code so I wouldn't use this as an argument against...
[1] https://bugzilla.wikimedia.org/show_bug.cgi?id=44130 [2]
https://github.com/jdlrobson/Minerva/blob/evenmorevanilla/Minerva.php#L72
[3]
https://github.com/jdlrobson/Minerva/blob/evenmorevanilla/minerva/templates/...
[4] http://www.gossamer-threads.com/lists/wiki/wikitech/355021
On Mon, May 13, 2013 at 3:27 PM, Yuri Astrakhan yastrakhan@wikimedia.org wrote:
On one hand, I prefer to have a properly formatted code, but on the
other,
most systems i have worked with have a very high cost of string concatenation, and I have a strong suspicion PHP is guilty of that too. Constructing HTML one element/value at a time might prove to be on of
the
bigger perf bottlenecks.
From my personal experience, once I worked on a networking lib for proprietary protocol, and noticed that there was a lot of logging calls
in
the form of Log("value1=" + value1 + " value2=" + value2 ...). After I switched it to the form "Log("value1={0}, value2={1}", value1, value2),
the
code became an order of magnitude faster because the logging framework deferred concatenation until the last moment after it knew
that
logging is needed, and the actual concatenation was done for the whole complex string with values, not one substring at a time.
On Mon, May 13, 2013 at 6:10 PM, Antoine Musso hashar+wmf@free.fr
wrote:
Le 13/05/13 19:26, Max Semenik a écrit :
Hi, I've seen recently a lot of code like this:
$html = Html::openElement( 'div', array( 'class' => 'foo' ) . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' =>
$somePotentiallyUnsafeId ),
$somePotentiallyUnsafeText ) ) . Html::closeElement( 'div' );IMO, cruft like this makes things harder to read and adds additional performance overhead.
Html is just like our Xml class, that let us raise the probability
that
the result code will be valid. That is also a good way to make sure
the
content is properly escaped, though in the example above that could
lead
to some mistake due to all the nested calls.
For the performance overhead, it surely exist but it is most probably negligible unless the methods are in a heavily used code path.
Ideally we would use templates to generate all of that. That will let
us
extract the views logic out of the PHP code.
-- Antoine "hashar" Musso
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
-- Jon Robson http://jonrobson.me.uk @rakugojon
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Le 14/05/13 02:23, Jon Robson a écrit :
Following on from Antoine's post, I experimented recently with using a template engine Mustache that works on both javascript and PHP and allows separation of HTML templates from PHP code.
Another template engine is Twig. It is used by the Silex micro engine (based on Symfony2). See: http://twig.sensiolabs.org/
Examples:
{{ foobar }} # not escaped {{ unsafevar|escaped }} # yeah protection!
You can iterate:
<ul id="users"> {% for user in users %} <li><a href="{{ user.href }}">{{ users.name }}</a></li> {% endfor %}
The problem is that it is just for PHP whereas Mustache has implementations in Javascript as well.
Hey,
Since no one complained about this yet in this thread: Html::element and associated methods are static! :/
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. --
On Tue, May 14, 2013 at 7:36 AM, Jeroen De Dauw jeroendedauw@gmail.com wrote:
Hey,
Since no one complained about this yet in this thread: Html::element and associated methods are static! :/
Well they're utility functions meant to be used from near anywhere, which I've always considered an acceptable usage of static functions.
-Chad
Hey,
Well they're utility functions meant to be used from near anywhere, which
I've always considered an acceptable usage of static functions.
One of the few places where static does not hurt is in leaf methods. For instance Math::abs. It is still somewhat dangerous to create such methods, since it is quite possible a leaf stops being a leaf. And Html::element certainly is not a leaf method. It is invoking other methods that on their turn invoke more methods, some of which with quite high complexity. And some of which are using global variables or static fields in the Html class. Using this nearly everywhere means that nearly everything is quite dependent on this specific code and its state. The principle that generally components with a lot of incoming dependencies should be more abstract then concrete is also violated here.
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. --
On 05/14/2013 07:54 AM, Jeroen De Dauw wrote:
One of the few places where static does not hurt is in leaf methods. For instance Math::abs. It is still somewhat dangerous to create such methods, since it is quite possible a leaf stops being a leaf. And Html::element certainly is not a leaf method. It is invoking other methods that on their turn invoke more methods, some of which with quite high complexity. And some of which are using global variables or static fields in the Html class. Using this nearly everywhere means that nearly everything is quite dependent on this specific code and its state.
What state? The only class state I see is configuration information (namely $voidElements, $boolAttribs, and $HTMLFiveOnlyAttribs, none of which are written too).
Those are the only static fields I'm aware of.
And I believe the only globals are simple configuration variables (e.g. wgWellFormedXml) from LocalSettings (nothing like $wgUser).
Matt Flaschen
Hey,
What state? The only class state I see is configuration information
(namely $voidElements, $boolAttribs, and $HTMLFiveOnlyAttribs, none of which are written too).
Those are the only static fields I'm aware of.
And I believe the only globals are simple configuration variables (e.g. wgWellFormedXml) from LocalSettings (nothing like $wgUser).
Indeed. That is still global state however. More complex global state would be worse. That does not make simple global state not harmful though.
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. --
On Tue, May 14, 2013 at 2:34 AM, Antoine Musso hashar+wmf@free.fr wrote:
Le 14/05/13 02:23, Jon Robson a écrit :
Following on from Antoine's post, I experimented recently with using a template engine Mustache that works on both javascript and PHP and allows separation of HTML templates from PHP code.
Another template engine is Twig. It is used by the Silex micro engine (based on Symfony2). See: http://twig.sensiolabs.org/
Examples:
{{ foobar }} # not escaped {{ unsafevar|escaped }} # yeah protection!
You can iterate:
<ul id="users"> {% for user in users %} <li><a href="{{ user.href }}">{{ users.name }}</a></li> {% endfor %}
I'll actually admit this is one reason why templating makes me nervous. DOM text, attribute values, and urls all need different validation and escaping, so you can't just look at the template and make sure everything has |e, nor can you look at the PHP and see that everything is escaped before being passed to the template. And looking at both and making sure that each variable in the output has been correctly escaped for the html context in the PHP is a lot more work than just seeing $output .= Html::element( ... ).
It looks like we can define custom filters in twig, so we may be able to move the review to making sure the template correctly escapes the value for the context with the correct function. Something like:
<ul id="users"> {% for user in users %} <li><a href="{{ user|getMediaWikiUserURL }}">{{ users.name|e }}</a></li> {% endfor %}
The problem is that it is just for PHP whereas Mustache has implementations in Javascript as well.
-- Antoine "hashar" Musso
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Tue, 14 May 2013 09:45:05 -0700, Chris Steipp csteipp@wikimedia.org wrote:
On Tue, May 14, 2013 at 2:34 AM, Antoine Musso hashar+wmf@free.fr
Another template engine is Twig. It is used by the Silex micro engine (based on Symfony2). See: http://twig.sensiolabs.org/
Examples:
{{ foobar }} # not escaped {{ unsafevar|escaped }} # yeah protection!
You can iterate:
<ul id="users"> {% for user in users %} <li><a href="{{ user.href }}">{{ users.name }}</a></li> {% endfor %}
I'll actually admit this is one reason why templating makes me nervous. DOM text, attribute values, and urls all need different validation and escaping, so you can't just look at the template and make sure everything has |e, nor can you look at the PHP and see that everything is escaped before being passed to the template. And looking at both and making sure that each variable in the output has been correctly escaped for the html context in the PHP is a lot more work than just seeing $output .= Html::element( ... ).
Same here. This is actually why along for my skin rewrite plans the template system I was working on for it is context sensitive. I've contemplated now and then about the idea of tweaking that idea so a similar context sensitive template system could be used in general code.
Chris Steipp csteipp@wikimedia.org wrote:
[...]
It looks like we can define custom filters in twig, so we may be able to move the review to making sure the template correctly escapes the value for the context with the correct function. Something like:
<ul id="users"> {% for user in users %} <li><a href="{{ user|getMediaWikiUserURL }}">{{ users.name|e }}</a></li> {% endfor %}
[...]
So new MediaWiki developers would have to learn yet another syntax, debugging would require another level of indirec- tion, and if these custom filters include those we use for HTML5 now, the output would subtly differ from the template. What was the expected benefit of this operation again, and how often do we get to reap it? :-)
Tim
Besides, it adds much more overhead. What should we favour: readability or perfomance?
On Wed, May 15, 2013 at 11:43 PM, Tim Landscheidt tim@tim-landscheidt.dewrote:
Chris Steipp csteipp@wikimedia.org wrote:
[...]
It looks like we can define custom filters in twig, so we may be able to move the review to making sure the template correctly escapes the value for the context with the correct function. Something like:
<ul id="users"> {% for user in users %} <li><a href="{{ user|getMediaWikiUserURL }}">{{ users.name|e
}}</a></li>
{% endfor %}
[...]
So new MediaWiki developers would have to learn yet another syntax, debugging would require another level of indirec- tion, and if these custom filters include those we use for HTML5 now, the output would subtly differ from the template. What was the expected benefit of this operation again, and how often do we get to reap it? :-)
Tim
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 13.05.2013 21:26, Max Semenik wrote:
Hi, I've seen recently a lot of code like this:
$html = Html::openElement( 'div', array( 'class' => 'foo' ) . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . Html::closeElement( 'div' );
IMO, cruft like this makes things harder to read and adds additional performance overhead. It can be simplified to
$html = '<div class="foo'><p>' . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . '</p></div>';
What's your opinion, guys and gals?
In my Extension:QPoll I implemented tag arrays, which are more compact and support auto-closing of tags (no closeElement() is needed): http://svn.wikimedia.org/viewvc/mediawiki/trunk/extensions/QPoll/includes/qp...
For performance reasons I did not bother about attribute validation and inner text quotation, performing these tasks in caller instead. Output generarion was simple recursive traversal of nested PHP arrays.
However, it also has methods to "dynamically" add columns and rows to html tables, including colspans and rowspans.
The class worked well enough allowing to manipulate content, however subtree inserting and moving was not so elegant.
When I forced to abandon MediaWiki development due to financial reasons, I re-thinked the class and made the same tagarrays based on XMLDocument and XMLWriter:
https://bitbucket.org/sdvpartnership/questpc-framework/src/a5482dd1035b6393f... https://bitbucket.org/sdvpartnership/questpc-framework/src/a5482dd1035b6393f...
They are slower than "echo $var;" for sure, but allow much more powerful tag manipulation and templating in jQuery-like way. And the output is always valid and of course XMLDocument automatically cares about inner text escaping and so on.
Here's an example of newer version of tag array definition: array( '@tag' => 'div', 'class' => 'foo', array( '@tag' => 'p', array( '@tag' => 'span', 'id' => $id, $text ) ) );
String keys starting from '@' are special keys: '@tag' is a tag name, '@len' (optional) is count of child nodes. Another string keys are attribute key / value pairs. Integer keys are nested nodes - either nested tags or text nodes. Also there are three special tag names: '@tag' => '#text' '@tag' => '#comment' '@tag'=>'#cdata'
Dmitriy
On 13.05.2013 21:26, Max Semenik wrote:
Hi, I've seen recently a lot of code like this:
$html = Html::openElement( 'div', array( 'class' => 'foo' ) . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . Html::closeElement( 'div' );
IMO, cruft like this makes things harder to read and adds additional performance overhead. It can be simplified to
$html = '<div class="foo'><p>' . Html::rawElement( 'p', array(), Html::element( 'span', array( 'id' => $somePotentiallyUnsafeId ), $somePotentiallyUnsafeText ) ) . '</p></div>';
What's your opinion, guys and gals?
With php 5.4+ array syntax the code will be even more compact (comparable to raw html but structured easily manipulated data) [ '@tag' => 'div', 'class' => 'foo', [ '@tag' => 'p', [ '@tag' => 'span', 'id' => $id, $text ], '#comment' => $comment ] ];
Placing language operators into template itself like
{% for user in users %}
does not separate html data from code well enough. Template language always will be more limited comparing to processing via PHP classes. Dmitriy
wikitech-l@lists.wikimedia.org