New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ditch or patch ezSQL #1544

Closed
ozh opened this Issue Nov 15, 2013 · 12 comments

Comments

Projects
None yet
3 participants
@ozh
Member

ozh commented Nov 15, 2013

When I started YOURLS, ezSQL was the defacto MySQL lib to use for coders with a WP background like myself.

Early in the 1.7 dev cycle I built on this and extended the use of ezSQL to offer support for MySQL, MySQLi and PDO, to prepare for MySQL deprecation.

However, some things have changed :

  • we'll now require PHP 5.2+ which will, I think, always include mysqli and pdo extension on 100% hosts, including low end shared hosting
  • ezSQL doesn't support prepared statements which would take care of SQL injections reports (#1475, #1531, #1532)
  • I contributed to ezSQL with a few patches recently and realized that its code is not as great as I used to think

So, I think now is the time to reconsider things.

Option 1 would be to patch ezSQL to support prepare methods, for at least one of mysqli or PDO

Option 2 would be to ditch ezSQL altogether:

  • adopt one MySQL extension, either mysqli or pdo (which one?)
  • use a 3rd party library to make things simpler? (which one?)
  • rewrite all functions that deal with SQL

Any thought, any one?

@Art4

This comment has been minimized.

Show comment
Hide comment
@Art4

Art4 Nov 18, 2013

Just my thought: What about using the DB class from FuelPHP (and originally from Kohana)? See here, here and the docs.

This will need some rewrites (especially config and exception handling), but it's possible and you'll have an db abstraction which allows you to switch easily between mysql, mysqli and pdo. Read more about the usage.

Art4 commented Nov 18, 2013

Just my thought: What about using the DB class from FuelPHP (and originally from Kohana)? See here, here and the docs.

This will need some rewrites (especially config and exception handling), but it's possible and you'll have an db abstraction which allows you to switch easily between mysql, mysqli and pdo. Read more about the usage.

@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Nov 18, 2013

Member

Thanks for the tip, I'll look into this. First thing I notice is it's PHP 5.3+, while we're aiming for 5.2+ since it has a very large user base

Member

ozh commented Nov 18, 2013

Thanks for the tip, I'll look into this. First thing I notice is it's PHP 5.3+, while we're aiming for 5.2+ since it has a very large user base

@Art4

This comment has been minimized.

Show comment
Hide comment
@Art4

Art4 Nov 18, 2013

@ozh PHP 5.3+ is required for FuelPHP altogether, maybe the DB part can be used with or rewritten for PHP 5.2.

Edit: Just reviewed the changes between 5.2 and 5.3. Rewriting won't be as easy as I thought first. :-/

Art4 commented Nov 18, 2013

@ozh PHP 5.3+ is required for FuelPHP altogether, maybe the DB part can be used with or rewritten for PHP 5.2.

Edit: Just reviewed the changes between 5.2 and 5.3. Rewriting won't be as easy as I thought first. :-/

@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Dec 15, 2013

Member

Current plan I have in mind :

  • implement the "phone home" feature to collect stats, and specifically what is the PDO user base (assuming 100%?)
  • convert to PDO (rewrite all queries) which will have the benefit of being usable with various DB drivers
Member

ozh commented Dec 15, 2013

Current plan I have in mind :

  • implement the "phone home" feature to collect stats, and specifically what is the PDO user base (assuming 100%?)
  • convert to PDO (rewrite all queries) which will have the benefit of being usable with various DB drivers

@ozh ozh closed this Dec 15, 2013

@LeoColomb

This comment has been minimized.

Show comment
Hide comment
@LeoColomb

LeoColomb Feb 1, 2014

Member

@ozh On garde ezSQL alors ou pas ?
Il y a aussi à ma connaisance Medoo.

Member

LeoColomb commented Feb 1, 2014

@ozh On garde ezSQL alors ou pas ?
Il y a aussi à ma connaisance Medoo.

@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Feb 1, 2014

Member

Je suis toujours un peu partagé là-dessus :)

J'ai revu ma position concernant les SQL injections (cf les 3 mentionnées ci-dessus): en fait utiliser une autre classe n'aurait rien changé, c'était de l'erreur de noob et des queries avec des trucs non sanitizés.

Par contre cela n'empêche pas qu'ezSQL manque de prepared statements et qu'il faudrait en avoir à disposition. Meedo a l'air pas mal mais semble un peu léger aussi à ce niveau là (pas vu de "prepare" dans le code + catfan/Medoo#44) mais c'est peut-être implémenté autrement

Quand j'aurai fini ma branche cron je ferai une nouvelle branche pour tester cette lib et voir quels pourraient être les bénéfices concrets (et voir de plus près pourquoi l'absence de "prepare")

Member

ozh commented Feb 1, 2014

Je suis toujours un peu partagé là-dessus :)

J'ai revu ma position concernant les SQL injections (cf les 3 mentionnées ci-dessus): en fait utiliser une autre classe n'aurait rien changé, c'était de l'erreur de noob et des queries avec des trucs non sanitizés.

Par contre cela n'empêche pas qu'ezSQL manque de prepared statements et qu'il faudrait en avoir à disposition. Meedo a l'air pas mal mais semble un peu léger aussi à ce niveau là (pas vu de "prepare" dans le code + catfan/Medoo#44) mais c'est peut-être implémenté autrement

Quand j'aurai fini ma branche cron je ferai une nouvelle branche pour tester cette lib et voir quels pourraient être les bénéfices concrets (et voir de plus près pourquoi l'absence de "prepare")

@LeoColomb

This comment has been minimized.

Show comment
Hide comment
@LeoColomb
Member

LeoColomb commented Feb 20, 2014

@LeoColomb

This comment has been minimized.

Show comment
Hide comment
@LeoColomb

LeoColomb Feb 20, 2014

Member

Et pourquoi pas un bon ORM tel que Doctrine ?

Member

LeoColomb commented Feb 20, 2014

Et pourquoi pas un bon ORM tel que Doctrine ?

@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Feb 21, 2014

Member

J'ai déjà regardé 2 ou 3 fois rapidement Doctrine, et à chaque fois je m'arrête au fait que ça ressemble à une ptin d'usine à gaz, que leur site de doc est horrible, et que des trucs genre Setup::createAnnotationMetadataConfiguration ça me fait vomir d'horreur. Assez instinctif comme jugement, je te l'accorde, mais j'ai pas l'impression que ça soit un truc simple et j'ai pas envie de perdre 30 jours à apprendre et maitriser une lib géante pour faire 12 queries.

J'ai re-jeté un oeil à FuelPHP par contre ça m'a l'air intéressant

Member

ozh commented Feb 21, 2014

J'ai déjà regardé 2 ou 3 fois rapidement Doctrine, et à chaque fois je m'arrête au fait que ça ressemble à une ptin d'usine à gaz, que leur site de doc est horrible, et que des trucs genre Setup::createAnnotationMetadataConfiguration ça me fait vomir d'horreur. Assez instinctif comme jugement, je te l'accorde, mais j'ai pas l'impression que ça soit un truc simple et j'ai pas envie de perdre 30 jours à apprendre et maitriser une lib géante pour faire 12 queries.

J'ai re-jeté un oeil à FuelPHP par contre ça m'a l'air intéressant

@LeoColomb

This comment has been minimized.

Show comment
Hide comment
@LeoColomb

LeoColomb Feb 21, 2014

Member

Hmm... oui je suis un peu d'accord pour Doctrine après petite étude.
FuelPHP ? Why not.

Member

LeoColomb commented Feb 21, 2014

Hmm... oui je suis un peu d'accord pour Doctrine après petite étude.
FuelPHP ? Why not.

@Art4

This comment has been minimized.

Show comment
Hide comment
@Art4

Art4 Feb 21, 2014

@LeoColomb fuel/orm depends on the FuelPHP DB class again. Take a look an fuelphp/database, it is ready to be used and requires Doctrine for some special operations.

And take a look an fuelphp/orm from FuelPHP 2.0. It depends on fuelphp/database, but is still in development.

Btw: Can you please use English, so I and other people don't have to use Google Translator? Also in other issues?

Art4 commented Feb 21, 2014

@LeoColomb fuel/orm depends on the FuelPHP DB class again. Take a look an fuelphp/database, it is ready to be used and requires Doctrine for some special operations.

And take a look an fuelphp/orm from FuelPHP 2.0. It depends on fuelphp/database, but is still in development.

Btw: Can you please use English, so I and other people don't have to use Google Translator? Also in other issues?

@LeoColomb

This comment has been minimized.

Show comment
Hide comment
@LeoColomb

LeoColomb Feb 21, 2014

Member

@Art4 Oh yeah, I've forgotten it's originally you proposal. Thanks again for the link and explanations. 😃
And sorry for french...

Member

LeoColomb commented Feb 21, 2014

@Art4 Oh yeah, I've forgotten it's originally you proposal. Thanks again for the link and explanations. 😃
And sorry for french...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment