Problems caused by long lines in views

Posted in October 2012 by under developer

When it comes to writing PHP, most people put no more than one command on a line. It is rare to see something like this in a controller.

$name = $this->userService->getName(); $name = (null == $name)?'':$name; return $name;

However when it comes to views, it is not uncommon to see something like

<p>You are logged in as <a href="/profile/<?php echo $this->escapeUrl($username);?>" ><php echo $this->escapeHtml($name);?><a><p>

instead of something like

    You are logged in as
    <a href="/profile/<php echo $this->escapeUrl($username);?> >
        <php echo $this->escapeHtml($user);?>

I am not sure why it is considered acceptable to have mutiple php commands in a long line in a view.

Recently, as part of testing an application with hundreds of views, I wrote a script that parses all view files and looks for unescaped output. This script produces a report of the filename, the line number and the unescaped code. It also uses git blame to look up the developer who wrote the line, just for fun!

There is output that is escaped that needs to be excluded from this report. I started to reduce the noise by excluding things like $this->escapeHtml and $this->url.

That on the face of it would work. However consider the line

<p>You are logged in as <a href="/profile/<?php echo $username;?>" ><php echo $this->escapeHtml($name);?></a><p>

You see the $username is not escaped but the line does contain $this->escapeHtml. So it becomes a lot more complicated to exclude known escaped output.

Another problem is when you get a pull request and the diff is like

+<p>View profile for <a href="<?php echo $this->url('userprofile',array('action' => 'view', 'user' => $username));?><?php echo $this->userProfile($username);?><a><p>
-<p>View profile for <a href="<?php echo $this->url('userprofile',array('action' => 'view', 'user' => $user->id));?><?php echo $this->userProfile($user->id);?><a><p>

It is not immediately obvious what the change is.

If this is the only diff in the pull request then things aren't so bad. But if you have many lines like this, it is not always easy to see what has changed. Especially if there are also pointless style changes to contend with.