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.
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.
Thanks! 1 - https://github.com/phpmyadmin/phpmyadmin/pull/290#issuecomment-17281963
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.
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.
Thanks! 1 - https://github.com/phpmyadmin/phpmyadmin/pull/290#issuecomment-17281963
On Sun, May 5, 2013 at 7:45 AM, Marc Delisle 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.
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
Get 100% visibility into Java/.NET code with AppDynamics Lite It's a free troubleshooting tool designed for production Get down to code-level detail for bottlenecks, with <2% overhead. Download for free and get started troubleshooting in minutes. http://p.sf.net/sfu/appdyn_d2d_ap2 _______________________________________________ Phpmyadmin-devel mailing list Phpmyadmin-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
Le 2013-05-06 09:46, Isaac Bennetch a écrit :
On Sun, May 5, 2013 at 7:45 AM, Marc Delisle <marc@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
On Mon, May 6, 2013 at 3:23 PM, Marc Delisle marc@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@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 theuser
> tries to insert to an invalid column type...so it looks broken inthe
> 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 columntype,
> however that isn't the most user-friendly idea. To keep users most > happy, I wondered if we should implement some sanity checks forsome
> 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 aboutthis,
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.
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 canbe
> 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
Learn Graph Databases - Download FREE O'Reilly Book "Graph Databases" is the definitive new guide to graph databases and their applications. This 200-page book is written by three acclaimed leaders in the field. The early access version is available now. Download your free book today! http://p.sf.net/sfu/neotech_d2d_may _______________________________________________ Phpmyadmin-devel mailing list Phpmyadmin-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
Isaac Bennetch a écrit :
On Mon, May 6, 2013 at 3:23 PM, Marc Delisle marc@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@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 theuser
> tries to insert to an invalid column type...so it looks broken inthe
> 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 columntype,
> however that isn't the most user-friendly idea. To keep users most > happy, I wondered if we should implement some sanity checks forsome
> 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 aboutthis,
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 canbe
> 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
-
Isaac Bennetch -
Marc Delisle