Begin work on proper MyBB Application and Extension system #272
Conversation
…vidor, and new settings repository
…g, and new models
|
@euantorano should be able to begin work on the installer soon, also this should make it easier to implement an api and separate interfaces for the forum, admin, and api |
|
Can you please fix the code style issues? You can ignore the JS one - I need to look at why it's checking JS... |
|
Yes, will do that now |
| /** | ||
| * Add default values for config values. | ||
| */ | ||
| public static function addSettings($defaults) |
There was a problem hiding this comment.
As these are arrays, can you type hint $defaults as array?
There was a problem hiding this comment.
added a hint as requested
| * @param Filesystem $files | ||
| * @param string $publicPath | ||
| */ | ||
| public function __construct(Filesystem $files, $publicPath) |
There was a problem hiding this comment.
We're requiring PHP7, so feel free to use scalar type hints - here for instance string $publicPath.
There was a problem hiding this comment.
added string hint as requested
| // First we will just make sure that there are any migrations to run. If there | ||
| // aren't, we will just make a note of it to the developer so they're aware | ||
| // that all of the migrations have been run against this database system. | ||
| if (count($migrations) == 0) { |
There was a problem hiding this comment.
Personally I'd use empty() here, but it's no big deal.
|
|
||
| $count = count($migrations); | ||
|
|
||
| if ($count === 0) { |
There was a problem hiding this comment.
Using count() makes sense here though, as the count is then used elsewhere.
| } | ||
|
|
||
| $files = array_map(function ($file) { | ||
| return str_replace('.php', '', basename($file)); |
There was a problem hiding this comment.
I would use pathinfo() here, and use the returned filename entry.
| */ | ||
| public function accessTokens() | ||
| { | ||
| return $this->hasMany('MyBB\Core\Database\Models\AccessToken'); |
There was a problem hiding this comment.
Again, I prefer \MyBB\Core\Database\Models\AccessToken::class.
| * @param string $model | ||
| * @return bool | ||
| */ | ||
| public function isModel($model) |
There was a problem hiding this comment.
Could this be renamed to modelClass or something, and use a scalar type hint? Calling it model may lead to some confusion thinking you can pass an object.
| */ | ||
| public function isModel($model) | ||
| { | ||
| return $this->model instanceof $model; |
| $mimetype = pathinfo($file, PATHINFO_EXTENSION) === 'svg' | ||
| ? 'image/svg+xml' | ||
| : finfo_file(finfo_open(FILEINFO_MIME_TYPE), $file); | ||
| $data = file_get_contents($file); |
There was a problem hiding this comment.
We should probably do some more proper validation to make sure it's actually an image here.
| $this->registerCache($app); | ||
|
|
||
| //todo: Register Local, Twig, Database, and other needed providors here | ||
| $app->register('MyBB\Core\Settings\SettingsServiceProvider'); |
There was a problem hiding this comment.
We should also probably load these from a config file or something, having them hardcoded here seems off to me.
There was a problem hiding this comment.
Fixed per request, it just registers the service provider at that part so we can use it.
|
Some of my above points still hand, especially the image validation. |
Added a class to properly register the MyBB instance, and creates base Extension class to be used by extensions.