On Mon, Jun 8, 2015 at 4:33 PM, Marc Delisle marc@infomarc.info wrote:
Le 2015-06-08 09:23, Dan Ungureanu a écrit :
Hello Marc,
`Parser::$STATEMENT_PARSERS` is a constant (its value remains the same during run-time), but I could not declare it as I should because it is illegal to declare constant arrays in versions prior to PHP 5.6. Also, I decided to avoid the `const` keyword in integer or string constants for consistency reasons. Do you think that I should rename the constant arrays and add the `const` keyword to other declarations?
Hi Dan, (please use bottom-posting on this list)
When you mention consistency, I am concerned with consistency with the phpMyAdmin codebase, which uses the 'const' keyword. Please use it, except for arrays.
On Mon, Jun 8, 2015 at 3:14 PM, Marc Delisle <marc@infomarc.info mailto:marc@infomarc.info> wrote:
Hi Dan, At this point, since you prefer to work in your own repository [0], I cannot add line comments via Github so I'll send my comments to this list. Please explain why in [1] you are using uppercase. This should beonly
for constants. [0] https://github.com/udan11/sql-parser [1]https://github.com/udan11/sql-parser/blob/master/src/Parser.php#L19
-- Marc Delisle | phpMyAdmin
_______________________________________________ Phpmyadmin-devel mailing list Phpmyadmin-devel@lists.sourceforge.net <mailto:Phpmyadmin-devel@lists.sourceforge.net> https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
Phpmyadmin-devel mailing list Phpmyadmin-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
-- Marc Delisle | phpMyAdmin
Phpmyadmin-devel mailing list Phpmyadmin-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
Hello Marc,
I changed declarations for constants.
Regarding the implementation of `sql-parser` in phpMyAdmin. I believe that the place of the library should be in the `libraries` directory among other libraries, but I am not sure if the tests should be in there as well. I see that no other library includes tests, but that might be just because they have no tests. Including tests would increase the size of the phpMyAdmin package with about 200kb (uncompressed) and I know that the team tries to keep the size of the package as small as possible.
The other question I have is about autoloading classes. For this library I used the autoloader that is generated by Composer. However, phpMyAdmin does not use Composer for managing dependencies and showed no interest in starting using it (I proposed it in the past), so now I have to find a way to include the required files. Possible solutions: a) manually include each class in each file; b) include all files in the beginning; c) use the generated Composer autoloader; d) write another smaller autoloader. I think that c) is the best solution, but I would love to hear your opinions as well.
Thank you!