<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, May 6, 2013 at 3:23 PM, Marc Delisle <span dir="ltr"><<a href="mailto:marc@infomarc.info" target="_blank">marc@infomarc.info</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Le 2013-05-06 09:46, Isaac Bennetch a écrit :<br>
<div class="im">><br>
> On Sun, May 5, 2013 at 7:45 AM, Marc Delisle <<a href="mailto:marc@infomarc.info">marc@infomarc.info</a><br>
</div><div><div class="h5">> <mailto:<a href="mailto:marc@infomarc.info">marc@infomarc.info</a>>> wrote:<br>
><br>
> Le 2013-05-04 10:54, Isaac Bennetch a écrit :<br>
> > Hi,<br>
> ><br>
> > Can I get some thoughts on [1]? The way the code exists<br>
> currently, you<br>
> > can insert bad data (you'll get a standard MySQL warning after the<br>
> > insert). For example, inserting the output of the MD5() function<br>
> in to a<br>
> > TINYINT column. This pull request implements the AES_ENCRYPT<br>
> function,<br>
> > but silently fails to show the salt form field in the event the user<br>
> > tries to insert to an invalid column type...so it looks broken in the<br>
> > case the user is trying to do something that's a bad idea.<br>
> ><br>
> > In keeping with the current system, I thought it should always<br>
> show the<br>
> > salt field and proceed with the insert regardless of the column type,<br>
> > however that isn't the most user-friendly idea. To keep users most<br>
> > happy, I wondered if we should implement some sanity checks for some<br>
> > column types and show an inline warning as needed (similar to the<br>
> > recently merged warning when creating a new user with the same<br>
> > username). Some of the functions require or suggest certain<br>
> column types<br>
> > which we could easily check, but I'm not sure it's phpMyAdmin's<br>
> > responsibility to constantly warn the user in this case.<br>
><br>
> I added a comment in the pull request. I also asked Garvin about this,<br>
> let's wait for his insight.<br>
<br>
</div></div>I got a reply from Garvin. He thinks that if the mapping between the<br>
function list and the columns type is actively maintained, there is no<br>
reason to show all possible functions in the drop-down.<br></blockquote><div><br></div><div>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."<br>
<br></div><div>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.<br></div><div><br></div><div>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. <br>
</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5">><br>
> ><br>
> > Regardless, the question at hand is whether this pull request can be<br>
> > merged as-is or if we should change the behavior in the case of an<br>
> > invalid column type.<br>
><br>
> If it works well for the correct column type, please merge.<br>
><br>
><br>
> My preference would be to change the behavior of the patch before<br>
> merging. I would prefer to see a small inline popup when attempting to<br>
> insert potentially invalid data, a second option I do not mind is simply<br>
> inserting the data and showing the MySQL warning about truncated data on<br>
> the results page (the current behavior for all functions except for this<br>
> patch). The least desirable behavior, which is implemented by this<br>
> patch, is to silently refuse to do anything on the insert page (in this<br>
> patch, AES_ENCRYPT requires a salt value which is prompted when<br>
> selecting the AES_ENCRYPT function; however if the column type is not<br>
> valid for AES_ENCRYPT data, the salt field silently fails to appear,<br>
> which I thought was a bug in the implementation).<br>
><br>
> I think for now I'll simply ask for the salt field to always appear<br>
> (matching current behavior), if we reach a decision about handling<br>
> known-invalid function/column combinations we can create a feature<br>
> request and move forward.<br>
><br>
> ><br>
> > Thanks!<br>
> > 1 -<br>
> <a href="https://github.com/phpmyadmin/phpmyadmin/pull/290#issuecomment-17281963" target="_blank">https://github.com/phpmyadmin/phpmyadmin/pull/290#issuecomment-17281963</a><br>
><br>
<br>
--<br>
Marc Delisle<br>
<a href="http://infomarc.info" target="_blank">http://infomarc.info</a><br>
<br>
------------------------------------------------------------------------------<br>
</div></div>Learn Graph Databases - Download FREE O'Reilly Book<br>
"Graph Databases" is the definitive new guide to graph databases and<br>
their applications. This 200-page book is written by three acclaimed<br>
leaders in the field. The early access version is available now.<br>
Download your free book today! <a href="http://p.sf.net/sfu/neotech_d2d_may" target="_blank">http://p.sf.net/sfu/neotech_d2d_may</a><br>
<div class="HOEnZb"><div class="h5">_______________________________________________<br>
Phpmyadmin-devel mailing list<br>
<a href="mailto:Phpmyadmin-devel@lists.sourceforge.net">Phpmyadmin-devel@lists.sourceforge.net</a><br>
<a href="https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel" target="_blank">https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel</a><br>
</div></div></blockquote></div><br></div></div>