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

Attempt at making bootstrap more modular #2294

Merged
merged 6 commits into from Oct 29, 2017

Conversation

Projects
None yet
2 participants
@ozh
Member

ozh commented Sep 13, 2017

The idea is to try and make the unit test bootstrapping less stupid with less duplicated code.

ozh added some commits Sep 13, 2017

Attempt at making bootstrap more modular
The idea is to try and make the unit test bootstrapping less stupid with less duplicated code.
Let's throw Exceptions on error
It will scare Average Joe even more, and is a bit more testable, although tests here are trivial
Fix docblock
[skip ci]
@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Sep 17, 2017

Member

Happy with it I think. @LeoColomb if you have a comment to send before I merge, now would be cool :)

Member

ozh commented Sep 17, 2017

Happy with it I think. @LeoColomb if you have a comment to send before I merge, now would be cool :)

@LeoColomb LeoColomb self-requested a review Sep 17, 2017

@LeoColomb

Here we go! Some comments and notes, feel free to use them as you want! 🙂

Oh and be careful, the coding style is not really coherent in the whole change, making it hard to read sometimes.

That said, globally this change is a big improvement!

Show outdated Hide outdated includes/YOURLS/Exceptions/Config.php
yourls_redirect( YOURLS_SITE .'/admin/upgrade.php', 302 );
}
}
require __DIR__ . '/vendor/autoload.php';

This comment has been minimized.

@LeoColomb

LeoColomb Sep 17, 2017

Member

👍

@LeoColomb

LeoColomb Sep 17, 2017

Member

👍

$init_defaults = new \YOURLS\Config\InitDefaults;
new \YOURLS\Config\Init($init_defaults);

This comment has been minimized.

@LeoColomb

LeoColomb Sep 17, 2017

Member

As Config\InitDefaults seems to be the defaults, why not directly use as default inside Config\Init?

@LeoColomb

LeoColomb Sep 17, 2017

Member

As Config\InitDefaults seems to be the defaults, why not directly use as default inside Config\Init?

This comment has been minimized.

@ozh

ozh Sep 18, 2017

Member

Well, this was my first go, but I found that ending up with a super long file was more cumbersome than dealing with two shorter ones.
Indeed this could be rewritten with something like this:

// In YOURLS :
$config = new \YOURLS\Config\Init();
$config->set_up();

// In YOURLS/YOURLS-Unit-Tests
$config = new \YOURLS\Config\Init();
$config->some_var = false;
$config->other_var = true;
$config->set_up();

I'll sleep over this. I kind of like this tiny InitDefaults class. :-)

@ozh

ozh Sep 18, 2017

Member

Well, this was my first go, but I found that ending up with a super long file was more cumbersome than dealing with two shorter ones.
Indeed this could be rewritten with something like this:

// In YOURLS :
$config = new \YOURLS\Config\Init();
$config->set_up();

// In YOURLS/YOURLS-Unit-Tests
$config = new \YOURLS\Config\Init();
$config->some_var = false;
$config->other_var = true;
$config->set_up();

I'll sleep over this. I kind of like this tiny InitDefaults class. :-)

/* The following require has to be at global level so the variables inside config.php, including user defined if any,
* are registered in the global scope. If this require is moved in \YOURLS\Config\Config, $yourls_user_passwords for
* instance isn't registered.
*/

This comment has been minimized.

@LeoColomb

LeoColomb Sep 17, 2017

Member

Have you tried something static? Static variable or function would allow a "global" access without hard loading, wouldn't it?

@LeoColomb

LeoColomb Sep 17, 2017

Member

Have you tried something static? Static variable or function would allow a "global" access without hard loading, wouldn't it?

This comment has been minimized.

@ozh

ozh Sep 18, 2017

Member

Not sure I get what you mean with "hard loading" ?

@ozh

ozh Sep 18, 2017

Member

Not sure I get what you mean with "hard loading" ?

This comment has been minimized.

@LeoColomb

LeoColomb Sep 25, 2017

Member

I mean loading variables as global like this.
if you load the file and store variables in a static variable of the Config class, you may be able to access it with a Config::variable (or something) everywhere.
You gain the ability to use methods and protect config.

@LeoColomb

LeoColomb Sep 25, 2017

Member

I mean loading variables as global like this.
if you load the file and store variables in a static variable of the Config class, you may be able to access it with a Config::variable (or something) everywhere.
You gain the ability to use methods and protect config.

* instance isn't registered.
*/
if (!defined('YOURLS_CONFIGFILE')) {
define('YOURLS_CONFIGFILE', $config->find_config());

This comment has been minimized.

@LeoColomb

LeoColomb Sep 17, 2017

Member

Is it still necessary to declare a YOURLS_CONFIGFILE const?

@LeoColomb

LeoColomb Sep 17, 2017

Member

Is it still necessary to declare a YOURLS_CONFIGFILE const?

This comment has been minimized.

@ozh

ozh Sep 18, 2017

Member

Yes because it's used in other places (such as includes/auth.php)
But at the same time, no, because constants make tests difficult so I'd rather replace most of them with function calls.
The problem here is backward compat because some const are widely used.

@ozh

ozh Sep 18, 2017

Member

Yes because it's used in other places (such as includes/auth.php)
But at the same time, no, because constants make tests difficult so I'd rather replace most of them with function calls.
The problem here is backward compat because some const are widely used.

This comment has been minimized.

@LeoColomb

LeoColomb Sep 25, 2017

Member

The problem here is backward compat because some const are widely used.

Ah, right.

@LeoColomb

LeoColomb Sep 25, 2017

Member

The problem here is backward compat because some const are widely used.

Ah, right.

namespace YOURLS\Config;
class InitDefaults {

This comment has been minimized.

@LeoColomb

LeoColomb Sep 17, 2017

Member

IMHO, this class is more an array that a real class.
By using a class we lose an eventual array merging with custom config values.

And if you prefer keeping variables, why not including it directly in the Config\Init class? This is weird! 😸

@LeoColomb

LeoColomb Sep 17, 2017

Member

IMHO, this class is more an array that a real class.
By using a class we lose an eventual array merging with custom config values.

And if you prefer keeping variables, why not including it directly in the Config\Init class? This is weird! 😸

This comment has been minimized.

@ozh

ozh Sep 18, 2017

Member

The idea of using an object instead of an array actually came from reading something like this, which I find rather elegant.

@ozh

ozh Sep 18, 2017

Member

The idea of using an object instead of an array actually came from reading something like this, which I find rather elegant.

This comment has been minimized.

@LeoColomb

LeoColomb Sep 25, 2017

Member

Haha, very good link! 😆
That said, even if I strongly agree with that good practice, I found the example not really adapted. What is the interest of creating a class to make an array with all public attributes? It cost more space and CPU than an array and is much more complicated to handle...

Edit: ok, you can check the type of parameters, but without deep type check that comes with php7 it's useless IMHO...

@LeoColomb

LeoColomb Sep 25, 2017

Member

Haha, very good link! 😆
That said, even if I strongly agree with that good practice, I found the example not really adapted. What is the interest of creating a class to make an array with all public attributes? It cost more space and CPU than an array and is much more complicated to handle...

Edit: ok, you can check the type of parameters, but without deep type check that comes with php7 it's useless IMHO...

This comment has been minimized.

@ozh

ozh Oct 8, 2017

Member

Interesting: replacing class Config\InitDefaults with an array within Config\Init and merging custom values (code) eats 6% more memory on my computer...

@ozh

ozh Oct 8, 2017

Member

Interesting: replacing class Config\InitDefaults with an array within Config\Init and merging custom values (code) eats 6% more memory on my computer...

require_once YOURLS_INC.'/functions-html.php';
require_once YOURLS_INC.'/functions-http.php';
require_once YOURLS_INC.'/functions-infos.php';
require_once YOURLS_INC.'/functions-deprecated.php';

This comment has been minimized.

@LeoColomb

LeoColomb Sep 17, 2017

Member

Side note for me, this can be enhanced to prettify errors.

array_map(function ($file_name) {
    $file = YOURLS_INC.'/'.$file_name.'.php';
    if (!file_exists($file)) {
        throw new FileException($file_name)
    }
    require_once YOURLS_INC.'/'.$file_name.'.php'
}, ['version', 'functions', 'functions-plugins', ...]);
@LeoColomb

LeoColomb Sep 17, 2017

Member

Side note for me, this can be enhanced to prettify errors.

array_map(function ($file_name) {
    $file = YOURLS_INC.'/'.$file_name.'.php';
    if (!file_exists($file)) {
        throw new FileException($file_name)
    }
    require_once YOURLS_INC.'/'.$file_name.'.php'
}, ['version', 'functions', 'functions-plugins', ...]);

This comment has been minimized.

@ozh

ozh Sep 18, 2017

Member

Not a fan of array_map() over foreach loop, I think it rarely makes the code easier to read. This said, throwing exception is an idea.

@ozh

ozh Sep 18, 2017

Member

Not a fan of array_map() over foreach loop, I think it rarely makes the code easier to read. This said, throwing exception is an idea.

}
}
}

This comment has been minimized.

@LeoColomb

LeoColomb Sep 17, 2017

Member

Wow, big constructor! 😆

Have you tried to make a loop with a call_user_func?

@LeoColomb

LeoColomb Sep 17, 2017

Member

Wow, big constructor! 😆

Have you tried to make a loop with a call_user_func?

public function define_core_constants() {
// Check minimal config job has been properly done
$must_haves = array('YOURLS_DB_USER', 'YOURLS_DB_PASS', 'YOURLS_DB_NAME', 'YOURLS_DB_HOST', 'YOURLS_DB_PREFIX', 'YOURLS_SITE');
foreach($must_haves as $must_have) {

This comment has been minimized.

@LeoColomb

LeoColomb Sep 17, 2017

Member

array_map?

@LeoColomb

LeoColomb Sep 17, 2017

Member

array_map?

@ozh ozh merged commit 1844291 into master Oct 29, 2017

3 of 4 checks passed

codacy/pr Not so good... This pull request quality could be better.
Details
Scrutinizer 1 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@LeoColomb LeoColomb deleted the bootstrap branch Nov 28, 2017

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