Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Begin work on proper MyBB Application and Extension system #272

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

xaoseric
Copy link
Contributor

Added a class to properly register the MyBB instance, and creates base Extension class to be used by extensions.

@xaoseric
Copy link
Contributor Author

@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

@euantorano
Copy link
Member

Can you please fix the code style issues? You can ignore the JS one - I need to look at why it's checking JS...

@xaoseric
Copy link
Contributor Author

Yes, will do that now

Copy link
Member

@euantorano euantorano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, though I'm not sure why you've added setting classes into the core? Also, some explanation of the roles of the new tables might be good, and some explanation of the overall architecture for future reference.

/**
* Add default values for config values.
*/
public static function addSettings($defaults)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As these are arrays, can you type hint $defaults as array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a hint as requested

* @param Filesystem $files
* @param string $publicPath
*/
public function __construct(Filesystem $files, $publicPath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're requiring PHP7, so feel free to use scalar type hints - here for instance string $publicPath.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I'd use empty() here, but it's no big deal.


$count = count($migrations);

if ($count === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using count() makes sense here though, as the count is then used elsewhere.

}

$files = array_map(function ($file) {
return str_replace('.php', '', basename($file));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use pathinfo() here, and use the returned filename entry.

*/
public function accessTokens()
{
return $this->hasMany('MyBB\Core\Database\Models\AccessToken');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I prefer \MyBB\Core\Database\Models\AccessToken::class.

* @param string $model
* @return bool
*/
public function isModel($model)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before here 😄

$mimetype = pathinfo($file, PATHINFO_EXTENSION) === 'svg'
? 'image/svg+xml'
: finfo_file(finfo_open(FILEINFO_MIME_TYPE), $file);
$data = file_get_contents($file);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, please use ::class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also probably load these from a config file or something, having them hardcoded here seems off to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed per request, it just registers the service provider at that part so we can use it.

@katosdev
Copy link

@xaoseric @euantorano

@euantorano
Copy link
Member

Some of my above points still hand, especially the image validation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants