Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-15 Thread Tim Landscheidt 
Chris Steipp cste...@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 %}
lia 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

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-15 Thread Paul Selitskas 
Besides, it adds much more overhead. What should we favour: readability or
perfomance?


On Wed, May 15, 2013 at 11:43 PM, Tim Landscheidt 
t...@tim-landscheidt.dewrote:

 Chris Steipp cste...@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 %}
 lia 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




-- 
З павагай,
Павел Селіцкас/Pavel Selitskas
Wizardist @ Wikimedia projects
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-14 Thread Antoine Musso 
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 %}
   lia 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.


-- 
Antoine hashar Musso

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-14 Thread Jeroen De Dauw 
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.
--
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-14 Thread Chad 
On Tue, May 14, 2013 at 7:36 AM, Jeroen De Dauw jeroended...@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

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-14 Thread Jeroen De Dauw 
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.
--
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-14 Thread Chris Steipp 
On Tue, May 14, 2013 at 2:34 AM, Antoine Musso hashar+...@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 %}
lia 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 %}
   lia 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

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-14 Thread Dmitriy Sintsov 

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 mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-14 Thread Matthew Flaschen 
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

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-14 Thread Jeroen De Dauw 
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.
--
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-14 Thread Daniel Friesen 
On Tue, 14 May 2013 09:45:05 -0700, Chris Steipp cste...@wikimedia.org  
wrote:



On Tue, May 14, 2013 at 2:34 AM, Antoine Musso hashar+...@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 %}
   lia 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.


--
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://danielfriesen.name/]


___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

[Wikitech-l] Code style: overuse of Html::element()

2013-05-13 Thread Max Semenik 
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

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-13 Thread Daniel Barrett 
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-boun...@lists.wikimedia.org 
[mailto:wikitech-l-boun...@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
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-13 Thread Chris Steipp 
On Mon, May 13, 2013 at 10:26 AM, Max Semenik maxsem.w...@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

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-13 Thread Tyler Romeo 
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 | tylerro...@gmail.com


On Mon, May 13, 2013 at 2:05 PM, Chris Steipp cste...@wikimedia.org wrote:

 On Mon, May 13, 2013 at 10:26 AM, Max Semenik maxsem.w...@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

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-13 Thread Paul Selitskas 
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 tylerro...@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 | tylerro...@gmail.com


 On Mon, May 13, 2013 at 2:05 PM, Chris Steipp cste...@wikimedia.org
 wrote:

  On Mon, May 13, 2013 at 10:26 AM, Max Semenik maxsem.w...@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




-- 
З павагай,
Павел Селіцкас/Pavel Selitskas
Wizardist @ Wikimedia projects
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-13 Thread Antoine Musso 
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

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-13 Thread Yuri Astrakhan 
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+...@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

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-13 Thread Jon Robson 
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/main.html
[4] http://www.gossamer-threads.com/lists/wiki/wikitech/355021

On Mon, May 13, 2013 at 3:27 PM, Yuri Astrakhan
yastrak...@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+...@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

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-13 Thread Matthew Walker 

 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 jdlrob...@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/main.html
 [4] http://www.gossamer-threads.com/lists/wiki/wikitech/355021

 On Mon, May 13, 2013 at 3:27 PM, Yuri Astrakhan
 yastrak...@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+...@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

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-13 Thread Jon Robson 
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 mwal...@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 jdlrob...@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/main.html
  [4] http://www.gossamer-threads.com/lists/wiki/wikitech/355021
 
  On Mon, May 13, 2013 at 3:27 PM, Yuri Astrakhan
  yastrak...@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+...@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' =

Re: [Wikitech-l] Code style: overuse of Html::element()

2013-05-13 Thread Dmitriy Sintsov 

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_renderer.php?revision=103452view=markup

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/a5482dd1035b6393f52049cda98c9539b6f77b6c/includes/Xml/XmlTree.php?at=master
https://bitbucket.org/sdvpartnership/questpc-framework/src/a5482dd1035b6393f52049cda98c9539b6f77b6c/includes/Xml/Writer/GenericXmlWriter.php?at=master

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


___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l