Re: [ppa-dev] why do we avoid printVal on null records in printTable

[JGuillaume (i. de R. <io...@fr...>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

JGuillaume (ioguix) de Rorthais a écrit :
> Robert Treat a écrit :
>> On Saturday 20 September 2008 16:57:22 Robert Treat wrote:
>>> Howdy folks,
>>>
>>> I was working on bug #1963050, regarding not showing database
>>> lists when you dont have connection privileges.  If you recall,
>>>  for 4.2.1, I fixed this by simply not returning a size for
>>> rows where you dont have permissions, because I thought
>>> something fancier might be a bit disruptive for a minor
>>> release. So, I was working on the fancier version in HEAD, and
>>> ran into some trouble with the following code (around line 1722
>>> of classes/Misc.php)
>>>
>>> if (!is_null($val)) { if (isset($column['url'])) { echo "<a
>>> href=\"{$column['url']}"; $misc->printUrlVars($column['vars'],
>>> $tabledata->fields); echo "\">"; }
>>>
>>> $type = isset($column['type']) ? $column['type'] : null;
>>>
>>> $params = isset($column['params']) ? $column['params'] :
>>> array();
>>>
>>>
>>> echo $misc->printVal($val, $type, $params); if
>>> (isset($column['url'])) echo "</a>"; }
>>>
>>> What throws me is the wrapper for !is_null($val). What I wanted
>>>  to do was return null for databases I didnt have permissions
>>> on, and then format the null with language specific text... the
>>>  problem is returning null means my value gets short-circuited
>>> by the above code. Now, I am inclined to remove this, but am a
>>> little concerned that something else might be relying on this
>>> behavior.  There shouldn't be imo, especially since printVal
>>> has machinery in place to handle nulls, and I see other parts
>>> of the code that try to make use of printVals null handling
>>> (unfortunatly i think i see some code trying to work around a
>>> lack of null handling too). Also, removing it in my patch makes
>>>  everything work like I had hoped. So, before I remove it, I
>>> thought I'd poke the list to see if anyone can recall why this
>>> was put in.
>>>
>>> *digs a bit*
>>>
>>> well, git blame tells me it was committed by ioguix, in this
>>> commit:
>>>
> http://github.com/xzilla/phppgadmin/commit/b782b46ecb0353e45a34b2f8a0582b6e
>
>>> d2fb952b
>>>
>>> that was quite a monster commit, but the relvant parts of
>>> classes/Misc.php seems to indicate he was trying to do some
>>> null protection or short circuiting, but I'm not sure why its
>>> needed... anyone want to chime in? ioguix?  (perhaps now would
>>> be a good time for me to try and get the regression tests
>>> working again...)
>> Just an FYI, but I did a little more looking, and have run the
>> selenium tests against my change, and everything seems fine, so I
>>  will probably commit this in the next couple days.
> Hello Robert,
>
> I quickly drop an eye on this commit. I hope I forget nothing, but
> here is my anderstanding about this code :
>
> This test (and the previous one) is here to test if the *field*
> exist in the recordset, not its value. The old test wasn't
> compatible with decorators and we are indeed using decorators in
> many place now. As you can guess, the field decorator return null
> if the field doesn't exist in the given result.
>
> But now, I wonder if we have a potential bug here. The field
> decorator returns the value for a given field name. So null could
> be a value OR means that the field doesn't exist in the record.
>
> So maybe you can have a look in the class FieldDecorator, method
> value, to secure that ? But i'm not sure it's in the scope of your
> patch and how to fix that...
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkjeujkACgkQxWGfaAgowiLFyQCbBwqIeC+FKUs+uBbMrKdK3c6+
QV4An3av3Ak2Gsu9BtpS2oYUNk9HpOhI
=gnWd
-----END PGP SIGNATURE-----