[Phpmyadmin-devel] Inserting known invalid data

[Marc Delisle]

Isaac Bennetch a écrit :
> On Mon, May 6, 2013 at 3:23 PM, Marc Delisle <marc at infomarc.info> wrote:
> 
>> Le 2013-05-06 09:46, Isaac Bennetch a écrit :
>>> On Sun, May 5, 2013 at 7:45 AM, Marc Delisle <marc at infomarc.info
>>> <mailto:marc at infomarc.info>> wrote:
>>>
>>>     Le 2013-05-04 10:54, Isaac Bennetch a écrit :
>>>      > Hi,
>>>      >
>>>      > Can I get some thoughts on [1]? The way the code exists
>>>     currently, you
>>>      > can insert bad data (you'll get a standard MySQL warning after the
>>>      > insert). For example, inserting the output of the MD5() function
>>>     in to a
>>>      > TINYINT column. This pull request implements the AES_ENCRYPT
>>>     function,
>>>      > but silently fails to show the salt form field in the event the
>> user
>>>      > tries to insert to an invalid column type...so it looks broken in
>> the
>>>      > case the user is trying to do something that's a bad idea.
>>>      >
>>>      > In keeping with the current system, I thought it should always
>>>     show the
>>>      > salt field and proceed with the insert regardless of the column
>> type,
>>>      > however that isn't the most user-friendly idea. To keep users most
>>>      > happy, I wondered if we should implement some sanity checks for
>> some
>>>      > column types and show an inline warning as needed (similar to the
>>>      > recently merged warning when creating a new user with the same
>>>      > username). Some of the functions require or suggest certain
>>>     column types
>>>      > which we could easily check, but I'm not sure it's phpMyAdmin's
>>>      > responsibility to constantly warn the user in this case.
>>>
>>>     I added a comment in the pull request. I also asked Garvin about
>> this,
>>>     let's wait for his insight.
>> I got a reply from Garvin. He thinks that if the mapping between the
>> function list and the columns type is actively maintained, there is no
>> reason to show all possible functions in the drop-down.
>>
> 
> The more I think about it, the more I prefer to show a warning message. Not
> showing all functions will likely confuse the user ("using table foo I see
> MD5() and AES_ENCRYPT(), with table bar I only see NOW()" ) -- so I propose
> that the appropriate way to resolve the situation is to always show all
> available functions, but to add a popup warning that "The function you have
> selected may not be compatible with the column type."
> 
> Doing so would require some method of mapping functions to compatible
> column types, but that list shouldn't change much so I don't see a problem
> with that.

We already have such mapping in libraries/Types.class.php.

> 
> What's everyone think? It seems to me the best of both worlds -- we're not
> changing the user experience and we're not blindly inserting data we know
> won't work.
> 
>>>      >
>>>      > Regardless, the question at hand is whether this pull request can
>> be
>>>      > merged as-is or if we should change the behavior in the case of an
>>>      > invalid column type.
>>>
>>>     If it works well for the correct column type, please merge.
>>>
>>>
>>> My preference would be to change the behavior of the patch before
>>> merging. I would prefer to see a small inline popup when attempting to
>>> insert potentially invalid data, a second option I do not mind is simply
>>> inserting the data and showing the MySQL warning about truncated data on
>>> the results page (the current behavior for all functions except for this
>>> patch). The least desirable behavior, which is implemented by this
>>> patch, is to silently refuse to do anything on the insert page (in this
>>> patch, AES_ENCRYPT requires a salt value which is prompted when
>>> selecting the AES_ENCRYPT function; however if the column type is not
>>> valid for AES_ENCRYPT data, the salt field silently fails to appear,
>>> which I thought was a bug in the implementation).
>>>
>>> I think for now I'll simply ask for the salt field to always appear
>>> (matching current behavior), if we reach a decision about handling
>>> known-invalid function/column combinations we can create a feature
>>> request and move forward.
>>>
>>>      >
>>>      > Thanks!
>>>      > 1 -
>>>
>> https://github.com/phpmyadmin/phpmyadmin/pull/290#issuecomment-17281963
>> --
>> Marc Delisle
>> http://infomarc.info



-- 
Marc Delisle
http://infomarc.info