WIP: Add Database Migrations (And more features as a result) #963
Conversation
… using bootstrappers
| @@ -0,0 +1,65 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Wouldn't these conflict with functions that might be defined by the underlying project already?
Especially as they are necessary for the functioning of the library.
Wouldn't it be better to have a dedicated Helpers class here?
There was a problem hiding this comment.
We can get rid of these helper functions, or wrap inside our namespace. I think its better than an abstract class with static functions
There was a problem hiding this comment.
or wrap inside our namespace
You mean as functions directly, instead of a class with static methods?
There was a problem hiding this comment.
Yes, like \Telegram\app().
Those helpers are for internal use, so in most cases, an user will not use those helpers
| try { | ||
| $this->createDotenv($app)->load(); | ||
| } catch (Exception $e) { | ||
| $this->writeErrorAndDie($e); |
There was a problem hiding this comment.
Does this make sense in respect of having this library integrated as part of an underlying project?
This literally forces the user to have a .env file (with that name!) in the current folder.
If I have a custom whatever.env that I load in my project, then the PHP Telegram Bot should just pick up those variables.
This bootstrapper should only be run when using the console command I think.
There was a problem hiding this comment.
If I have a custom whatever.env that I load in my project, then the PHP Telegram Bot should just pick up those variables.
How we should detect if user uses some custom whatever.env file?
There was a problem hiding this comment.
Well, that's what the checks in the Telegram constructor are for, right?
If no API key is given, then an exception is thrown.
Same thing happens for the DB.
I think libraries that extend functionality of a project shouldn't require an own .env setup, but instead should define which environment variables are necessary for it to work.
There was a problem hiding this comment.
@noplanman when user will create an instance of out Telegram/Application, our script will check the project's root for .env file.
I think libraries that extend functionality of a project shouldn't require an own .env setup, but instead should define which environment variables are necessary for it to work.
How to achieve this? Do not parse .env and just try to find variables in the $_ENV? Then user should parse .env file himself
| return $_ENV['APP_CONFIG_CACHE'] ?? $this->bootstrapPath() . '/cache/config.php'; | ||
| } | ||
|
|
||
| public function loadEnvironmentFrom($file): Application |
There was a problem hiding this comment.
This method can never be used, as the bootstrapping of the environment file happens in the constructor 🤔
How would one define a custom environment file as it is now?
There was a problem hiding this comment.
I am still thinking about that. How we can give the possibility to users to use some custom env file.
|
What about exporting all DB functionality to a separate package altogether? I'm just wondering how best to handle custom .env files for an existing project without creating any dependency issues, i.e. main project uses different version of phpdotenv. |
|
I am against the adoption of this PR, because it is a general-purpose package (CORE), and we must continue to develop it in this direction and not depend on implementation. If you need to integrate it into the framework, then create your own separate package for this, which will provide the necessary integration in a particular ecosystem. The same applies to creating your own framework based on this package. |
In this PR added:
php telegram blablaThis brings only one BC change, PHP version should be >=7.1.3, but versions below 7.1 are already deprecated and not supported anymore.
@noplanman @jacklul please take a look, what do you think about the changes
How to test:
Pull this branch, copy
.env.exampleto.envand write your settings, after runphp telegram migrateTwo tables should be created,
migrationsfor holding versions anduserstable which just copied from Laravel's default migration.