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

[+] Core : Implement Module Manager #4817

Merged
merged 38 commits into from Mar 15, 2016

Conversation

Projects
None yet
7 participants
@Quetzacoalt91
Member

Quetzacoalt91 commented Jan 29, 2016

Description coming soon

@Quetzacoalt91 Quetzacoalt91 added the WIP label Jan 29, 2016

@Quetzacoalt91 Quetzacoalt91 changed the title from [+] WIP : Implement Module Manager to [+] Core : Implement Module Manager Jan 29, 2016

_this.setNewDisplay($(this), '-grid', '-list');
});
// Change module addons item
addonItem.removeClass();
addonItem.addClass('module-addons-item-list col-md-12');
addonItem.addClass('module-addons-item-list col-lg-12');
this.setNewDisplay(addonItem, '-grid', '-list');
} else {
console.error('Can\'t switch to undefined display property "' + switchTo + '"');

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

What do you think about rename switchTo to displayProperty ?

@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

What do you think about rename switchTo to displayProperty ?

if (empty($module_errors)) {
if (!$moduleManager->install($module_name)) {
/*$module_errors = $module->getErrors();
if (empty($module_errors)) {*/

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

is that intended ?

@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

is that intended ?

return ($this->manage_categories && $this->manage_modules);
}
private function getModuleCache($file, $check_freshness = true)
{

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

I need more time to review this file ... I think too much responsabilities for only one class, did you take a look at the Theme implementation with Manager and Repository ?

@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

I need more time to review this file ... I think too much responsabilities for only one class, did you take a look at the Theme implementation with Manager and Repository ?

class ModuleDataProvider
{
public function findByName($name)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

this class should be called ModuleRepository regarding the consistency with Theme implementation.

@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

this class should be called ModuleRepository regarding the consistency with Theme implementation.

@@ -18,12 +21,21 @@ class ModuleController extends FrameworkBundleAdminController
*/

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

ModuleRepository could be (should be ?) declared as a service.

@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

ModuleRepository could be (should be ?) declared as a service.

@@ -1,15 +1,15 @@
<div id="module-modal-read-more-{{module.name}}{{ additionalModalSuffix|default('') }}" class="modal modal-vcenter fade" role="dialog">
<div id="module-modal-read-more-{{module.attributes.get('name')}}{{ additionalModalSuffix|default('') }}" class="modal modal-vcenter fade" role="dialog">
<div class="modal-dialog module-modal-dialog">

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

How about a ModulePresenter able to return exploitable representation in front ?

@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

How about a ModulePresenter able to return exploitable representation in front ?

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Mar 11, 2016

Member

Interesting idea, we will introduce this new class later as an improvement !

@Quetzacoalt91

Quetzacoalt91 Mar 11, 2016

Member

Interesting idea, we will introduce this new class later as an improvement !

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Mar 9, 2016

Contributor

Tests need to be done, by the way can't wait to be able to use this implementation :)

But... how about the "sandbox" ?I don't retrieve a safety mechanism to disable module in case of Exception.

Contributor

mickaelandrieu commented Mar 9, 2016

Tests need to be done, by the way can't wait to be able to use this implementation :)

But... how about the "sandbox" ?I don't retrieve a safety mechanism to disable module in case of Exception.

Quetzacoalt91 and others added some commits Mar 10, 2016

julienbourdeau added a commit that referenced this pull request Mar 15, 2016

@julienbourdeau julienbourdeau merged commit fe09c5b into PrestaShop:develop Mar 15, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maximebiloe

This comment has been minimized.

Show comment
Hide comment
@maximebiloe
Contributor

maximebiloe commented Mar 15, 2016

giphy

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