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

Code styling to output HTML in PHP #240

Open
lucymtc opened this issue Jul 3, 2018 · 14 comments
Open

Code styling to output HTML in PHP #240

lucymtc opened this issue Jul 3, 2018 · 14 comments

Comments

@lucymtc
Copy link

lucymtc commented Jul 3, 2018

We currently have a section to Avoid Heredoc and Nowdoc as it's impossible to do late escaping and where it's mentioned "engineers should avoid heredoc/nowdoc syntax and use traditional string concatenation & echoing instead".
I would like to know how people feel about how to construct HTML within the PHP code, echoing vs open/close php tags to include the HTML.

Across projects I see both different ways of doing it, I assume because it's a matter of personal preference.

But I do think

?>
<select>
	<option value="<?php esc_attr_e( $value ) ?>"><?php esc_attr_e( $option ) ?></option>
	<option value="<?php esc_attr_e( $value ) ?>"><?php esc_attr_e( $option ) ?></option>
	<option value="<?php esc_attr_e( $value ) ?>"><?php esc_attr_e( $option ) ?></option>
</select>
<?php

is more readable than echoing

echo '<select name="example">';
echo '<option value="' . esc_attr( $value ) . '">"' . esc_attr( $option ) . '"</option>';
echo '<option value="' . esc_attr( $value ) . '">"' . esc_attr( $option ) . '"</option>';
echo '<option value="' . esc_attr( $value ) . '">"' . esc_attr( $option ) . '"</option>';
echo '</select>';

Also in the way code editors highlight different code languages.
Although this is a very basic example, I have seen cases where it can get very messy and it would be nice to have some kind of unification across projects.

@dana-ross
Copy link
Contributor

I'm not a fan of jumping in & out of PHP with <?php and ?> from a stylistic standpoint. Mixing languages like that never sat well with me. I don't know, maybe it's just an uncomfortable reminder of PHP's past as a templating language.

My preference is for good old printf and sprintf:

printf( trim( '
<select>
	<option value="%s">%s</option>
	<option value="%s">%s</option>
	<option value="%s">%s</option>
</select>' ),
	esc_attr_e( $value ), esc_attr_e( $option ),
	esc_attr_e( $value ), esc_attr_e( $option ),
	esc_attr_e( $value ), esc_attr_e( $option )
);

This allows for a template-like syntax and late escaping.

@dana-ross
Copy link
Contributor

dana-ross commented Jul 3, 2018

In terms of unification, I get that it can sometimes be confusing to see different styles, but this isn't the kind of thing I think we should be dictating for our engineers. Like you said, it's largely a matter of personal preference.

If we did decide to standardize here, I'd want us to strongly consider adopting a lightweight templating library like Twig which would have additional benefits like automatic escaping.

@lucymtc
Copy link
Author

lucymtc commented Jul 5, 2018

That makes sense, but still in absence of a templating library maybe encouraging engineers to use printf and sprintf which still would keep a more clean and readable code vs echoing each line? From the different ways to output HTML I see across projects some of them can lead to messy code.

@ryanwelcher
Copy link
Contributor

+1 from me on recommending using printf or sprintf instead of echo when possible.

@krisgale-zz
Copy link

krisgale-zz commented Jul 5, 2018

respectfully, -1 from me, as it muddies up the works for esc_html__ and esc_html_e, as necessitated by i18n.

as for 'jumping in and out of php tags', consider that it at least tends to promote html looking like html... <a href="<?php esc_url( $some_url ); ?>">and so forth</a> -- this is quite readable.

@ryanwelcher
Copy link
Contributor

@krisgale you can ( and should ) use esc_html__/esc_html_e with those functions. Can you provide an example? Perhaps I'm misunderstanding?

@krisgale-zz
Copy link

krisgale-zz commented Jul 5, 2018

per the above example from @daveross , i see how it Can still be used with printf/sprintf -- i just don't see the real value of Additional function-wrapping.

@daveross also per your example, how might that look in a loop? like this?

<select>
<?php
for( $options as $option => $value ) {
  printf( trim( '
      <option value="%s">%s</option>
    ' ),
    esc_attr__( $value ), esc_attr__( $option )
  );
}
?>
</select>

how is this ^ better than...

<select>
<?php for( $options as $option => $value ) { ?>
  <option value="<?php esc_attr_e( $value ); ?>"><?php esc_html_e( $value ); ?></option>
<?php } ?>
</select>

?

to me one resembles Perl programming, the other resembles html with tags to output values.

@ryanwelcher
Copy link
Contributor

ryanwelcher commented Jul 5, 2018

I see what you mean :)

I think overall, this is a case of where the code is being used. If we have a template that is predominately HTML, I can see the benefit of the inline style of outputting PHP variables. It does read easier.

Ultimately, this is going to be very much the personal preference of the engineer. Maybe if this is included in the Best Practices, we recommend against the echo :allthethings approach, suggest the alternatives and leave it there?

@krisgale-zz
Copy link

i would agree that using printf/sprintf to assemble a bunch of html to prepare for output all-at-once further down in the page/template makes sense... with the caveat of being wary of the amount of markup being constructed and memory limitations.

@ryanwelcher
Copy link
Contributor

ryanwelcher commented Jul 5, 2018

@krisgale @daveross - we should be careful about getting too close to a particular use-case/example for this. There are going to be lots of examples on both sides.

@krisgale-zz
Copy link

krisgale-zz commented Jul 5, 2018

and react renders both obsolete :-p -- but that's a separate debate.

@barryceelen
Copy link
Member

barryceelen commented Jul 5, 2018

My 2 cents, for single line stuff that gets unreadably long I use (s)printf. When using (s)printf you can put the variables on their own lines which increases readability as opposed to the echo example.

printf(
	'<span><a href="%s" class="%" data-foo="%s" data-bar="%s">%s</a></span>',
	esc_url( $url ),
	esc_attr( $a_list_of_classes ),
	esc_attr( $foo ),
	esc_attr( $bar )
);

For html spanning a couple of lines I'd include a template file (bonus points for being able to be overridden by a child theme if that applies). For repeatable items, like in the <select> example I'd go for a loop. Code formatting wars 💣 !

@tlovett1
Copy link
Member

tlovett1 commented Aug 7, 2018

I think it really just depends on the amount of HTML being outputted. For a line our two surrounded by PHP, sprintf/printf makes sense. For a large block of HTML, a template file our <?php ?> makes sense to me. I agree this is pretty much personal preference.

@moraleida
Copy link

Somewhat tangent to this: if we were to standardize it, I'd prefer the standard to focus more on making sure we minimize the amount of logic needed on templates (by using helper functions to build all data before sending to the template) and the amount of markup built outside of templates. (we already have wording on this, but it could be expanded/more clear)

Then we could possibly recommend styles that align better in each case (I'd be in favor of @tlovett1's suggestion above)

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

No branches or pull requests

7 participants