On Sun, May 5, 2013 at 7:45 AM, Marc Delisle <marc(a)infomarc.info
<mailto:marc@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.
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