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

Remove ezSQL, use PDO #2282

Merged
merged 32 commits into from Sep 5, 2017

Conversation

Projects
None yet
2 participants
@ozh
Member

ozh commented Aug 18, 2017

Work in progress

Todo:

  • remove ezSQL, use PDO functions with the Aura.SQL wrapper lib. Woot.
  • make all \YOURLS\Database\YDB properties protected
  • write ezSQL compatible functions ($ydb->get_results() and such), mark them deprecated
  • rewrite all SQL queries to use PDO
  • pay attention to all @todo and @tempnote left in the process
  • Write SQL upgrade docs in the wiki

This is not related to PDO, but for the lulz, option function logic was moved to its own class, to leave very simple wrapper functions instead.

In the end, next to /includes/vendor, we should have /includes/YOURLS with a few classes for most complex functions

Regarding wrapper functions vs class functions

The idea is to :

  • make wrapper functions as small & concise as possible
  • keep the complex logic (validation, tests...) out of sight, in the class
  • keep return filters inside wrappers
  • move internal filters / actions in the class

Typical yourls_ function eventually, in /includes/functions*.php :

function yourls_do_stuff($var) {
    // Allow plugins to short-circuit options
    $pre = yourls_apply_filter( 'shunt_wrapper_for_stuff', false, $var );
    if ( false !== $pre )
        return $pre;

    global $ydb;
    $value = \YOURLS\Do\Stuff($ydb, $var, $bleh);

    return yourls_apply_filter('do_stuff', $value);
}

ozh added some commits Aug 18, 2017

1st pass at removing ezSQL. And an Options object.
Work in progress. This won't work for now.
Change autoloader for YOURLS stuff
It was OK in YOURLS itself, but not the unit tests since they are bypassing load-yourls.php
Var $option (and low level functions) in YDB
Keep $option low level functions and var in YDB, so new \Option don't start from zero each time and actually read value from $ydb instead of getting from DB each time.

Also, enforce utf8 at connection, is this a good idea? Test units will tell, later.

Also, more stuff here and there.
Plugins
I think in the end we could have a couple nice objects (Plugins/API.php, Plugins/Files.php and Plugins/Pages.php) to take all the complex logic out of functions-plugins.php

[skip ci]
More typos.
Thanks, scrutinizer-ci 😗

[skip ci]
Options sanity checks
As is, all current test units for options pass

[skip ci]
@LeoColomb

This comment has been minimized.

Show comment
Hide comment
@LeoColomb

LeoColomb Aug 24, 2017

Member

PDO functions with the Aura.SQL wrapper lib

Why choosing Aura.Sql? 😉

all SQL queries to use PDO

Actually this is why I don't understand some of your changes: since we're talking about database wrappers refactoring, why not abstracting queries and objects? Then the code could be fully independent of the driver.

move internal filters / actions in the class

You're moving to decorators and observers, right?

Member

LeoColomb commented Aug 24, 2017

PDO functions with the Aura.SQL wrapper lib

Why choosing Aura.Sql? 😉

all SQL queries to use PDO

Actually this is why I don't understand some of your changes: since we're talking about database wrappers refactoring, why not abstracting queries and objects? Then the code could be fully independent of the driver.

move internal filters / actions in the class

You're moving to decorators and observers, right?

@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Aug 24, 2017

Member

@leo :

  • Aura.SQL: well, why not. I looked into Doctrine and DBAL, I didn't wanted to add 800 KB of files for features I won't use, I even considered just using plain PDO, but found that Aura has a minimal set of interesting features to make things easier. It's merely PDO + just a few extra things.
  • Abstraction : at the moment, I'm focusing on making something fully compatible with the existing code to make sure no 3rd party stuff (plugins, public shorteners, whatever tool using YOURLS) will break. So basically I want all the old $ydb->get_row() or any other ezSQL legacy function work (albeit throwing a "deprecated" notice). Then I'll rewrite all YOURLS queries, in particular using PDO binding : at this step yes abstracting things would be nice, although I didn't really think about it yet. If you have any idea or POC on how it could be intelligently done, I'm all ears :)
  • Decorators and observers: I have no idea what you're talking about... Again, any code piece to illustrate what you mean is welcome.
Member

ozh commented Aug 24, 2017

@leo :

  • Aura.SQL: well, why not. I looked into Doctrine and DBAL, I didn't wanted to add 800 KB of files for features I won't use, I even considered just using plain PDO, but found that Aura has a minimal set of interesting features to make things easier. It's merely PDO + just a few extra things.
  • Abstraction : at the moment, I'm focusing on making something fully compatible with the existing code to make sure no 3rd party stuff (plugins, public shorteners, whatever tool using YOURLS) will break. So basically I want all the old $ydb->get_row() or any other ezSQL legacy function work (albeit throwing a "deprecated" notice). Then I'll rewrite all YOURLS queries, in particular using PDO binding : at this step yes abstracting things would be nice, although I didn't really think about it yet. If you have any idea or POC on how it could be intelligently done, I'm all ears :)
  • Decorators and observers: I have no idea what you're talking about... Again, any code piece to illustrate what you mean is welcome.

ozh added some commits Aug 28, 2017

Backward compatibility finished.
As of now, all unit tests (some slightly rewritten) pass.
Now, onto rewriting queries

[skip ci]

ozh added some commits Sep 3, 2017

Connect to DB before getting attribute
Attempt to fix the "PHP Fatal error:  Uncaught exception 'PDOException' with message 'SQLSTATE[IM001]: Driver does not support this function: driver does not support that attribute'" thrown on PHP 5.4. It could be that issuing PDO::getAttribute() before being connected to the DB prevents exception from being caught
More PDO::getAttribute wrapping
Damn you PHP 5.4
Remove leftover @todo note
[skip ci]
@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Sep 3, 2017

Member

All unit tests pass, f* yeah.
300full

Member

ozh commented Sep 3, 2017

All unit tests pass, f* yeah.
300full

@ozh ozh merged commit 10a8d98 into master Sep 5, 2017

1 of 2 checks passed

codacy/pr Not so good... This pull request quality could be better.
Details
Scrutinizer 7 new issues, 61 updated code elements
Details

@ozh ozh deleted the aurasql branch Sep 5, 2017

@LeoColomb

This comment has been minimized.

Show comment
Hide comment
@LeoColomb

LeoColomb Sep 5, 2017

Member

C'est pas facile de bosser avec toi en collab', j'aurai bien aimé relire avant la merge ! 😉

Member

LeoColomb commented Sep 5, 2017

C'est pas facile de bosser avec toi en collab', j'aurai bien aimé relire avant la merge ! 😉

@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Sep 6, 2017

Member

@LeoColomb arf désolé je pensais pas que tu suivais :)

Member

ozh commented Sep 6, 2017

@LeoColomb arf désolé je pensais pas que tu suivais :)

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