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

Be able to declare modern controllers in modules #9153

Merged
merged 4 commits into from Jun 25, 2018

Conversation

Projects
None yet
6 participants
@mickaelandrieu
Contributor

mickaelandrieu commented May 31, 2018

Questions Answers
Branch? 1.7.5
Description? After forms, templates, cli commands, now Controllers hip hip hip !!! Also, you'd better read this. I've ended up stealing the work of @sarjon :p
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
How to test? I'll provide a module soon. I'll add tests. (done). The module is named "demo" and is located in tests/resources/module: install and then access (after being logged in to the back office) to index.php/modules/demo/index. You should see the very first Symfony controller that comes from a module.

Something important for the future is missing here: a "modern" way to add a new Tab in the menu. This is the very first step of the system and everyone is welcome to suggest the next improvements.

==> Docs <==


This change is Reviewable

@mickaelandrieu mickaelandrieu changed the base branch from 1.7.4.x to develop May 31, 2018

@@ -1,3 +1,9 @@
app:
# The main bundle is PrestaShopCoreBundle which will load other dependencies.
resource: "@PrestaShopBundle/Resources/config/routing.yml"
admin_modules:

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

"pre-review" of @sarjon: "should be app_modules

I'll update :trollface:

arguments:
- '@=service("prestashop.module_kernel.repository").getActiveModulesPaths()'
- '@=service("prestashop.adapter.legacy.configuration").get("_PS_MODULE_DIR_")'
tags: [routing.loader]

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

missing end of line

class ModuleLoader extends Loader
{
/**
* @var array $activeModules The list of activated modules.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

activated modules paths

{
return 'module' === $type;
}
}

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

end of line :/

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 31, 2018

@sarjon your demonstration module should works as expected :trollface:

# v1: only YAML format is supported for now.
resource: .
type: module
prefix: /modules

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

I fear this may conflict with routes we have already... but as I'm working on better application routing, this won't be an issue in 1.7.5.

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jun 1, 2018

Member

In your comment, is foo the name of the module added automatically?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 1, 2018

Contributor

no, but I should replace foo by "module-name"

private $isLoaded = false;
/**
* @var string $modulePath The path to the modules folder.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

i dont need this

class: 'PrestaShopBundle\Routing\ModuleLoader'
arguments:
- '@=service("prestashop.module_kernel.repository").getActiveModulesPaths()'
- '@=service("prestashop.adapter.legacy.configuration").get("_PS_MODULE_DIR_")'

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

i dont need this

public function __construct(array $activeModulesPaths, $modulePath)
{
$this->activeModulesPaths = $activeModulesPaths;
$this->modulePath = $modulePath;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

i dont need this

@mickaelandrieu mickaelandrieu changed the title from Be able to declare modern controllers in modules to [WIP] Be able to declare modern controllers in modules May 31, 2018

public function __construct(array $activeModulesPaths, $modulePath)
{
$this->activeModulesPaths = $activeModulesPaths;

This comment has been minimized.

@sarjon

sarjon Jun 1, 2018

Member

i think we should be able to access installed but disabled module's controllers as well, since they can be configured. what do you think?

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jun 1, 2018

Member

Nah, we shouldn't load them in my opinion, because the configuration page of a module is basically not in a controller.

$this->module->onUninstall();
}
public function testRoutesAreRegistered()

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 1, 2018

Contributor

ping @sarjon, wdyt?

This comment has been minimized.

@sarjon

sarjon Jun 1, 2018

Member

seems good. 👍

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 4, 2018

Contributor

Not that good, the name of test file is misleading: how about YamlRoutesInModuleTest.php instead?

$this->name = 'demo';
$this->tab = 'front_office_features';
$this->version = 1.0;
$this->author = 'Šarūnas Jonušas';

This comment has been minimized.

@sarjon

sarjon Jun 1, 2018

Member

😃

<h1>Modern module Controller</h1>
<p>Awesome !</p>
{% endblock %}

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 1, 2018

Contributor

Missing \n

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 25, 2018

Contributor

fixed

@@ -0,0 +1,21 @@
Copyright (c) Nils Adermann, Jordi Boggiano

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 1, 2018

Contributor

Nils Adermann? Jordi Boggiano?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 1, 2018

Contributor

Yes, it's Composer License: do you ask me to steal Composer work?

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 1, 2018

Contributor

Can you add a README under demo module just to explain why there're these files :D

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 1, 2018

Contributor

are you kidding? xD it's vendor folder, there is nothing special with this module.

What do you suggest as README content?

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 4, 2018

Contributor

Just say something like "Demo module, files automatically generated by composer", a simple sentence for guys don't know tests / composer / vendor directories :)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 4, 2018

Contributor

Ok, fair enough :)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 25, 2018

Contributor

fixed btw

@@ -140,4 +140,9 @@ protected function enableDemoMode()
self::$kernel->getContainer()->set('prestashop.adapter.legacy.configuration', $configurationMock);
}
protected function clearCache()

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 4, 2018

Contributor

useless, to be removed.

@mickaelandrieu mickaelandrieu changed the title from [WIP] Be able to declare modern controllers in modules to Be able to declare modern controllers in modules Jun 5, 2018

@mickaelandrieu mickaelandrieu removed the wip label Jun 5, 2018

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 6, 2018

@PierreRambaud this contribution may be approved to, comments addressed 👍

Thanks for the review

@kpodemski

This comment has been minimized.

Contributor

kpodemski commented Jun 6, 2018

Hey @mickaelandrieu

what you mean here:

Something important for the future is missing here: a "modern" way to add a new Tab in the menu. This is the very first step of system and everyone is welcome to suggest the next improvements.

don't we have a way to do that by setting up $tabs property in Module class (created by @Quetzacoalt91)? it's ready from 1.7.2, or you meant "different" menu?

EDIT: Oh, right, entire mechanism depends on "regular" controllers (https://github.com/PrestaShop/PrestaShop/blob/develop/src/Adapter/Module/Tab/ModuleTabRegister.php#L160), you're right, and yes, this is very important

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 6, 2018

@kpodemski I can miss a lot of features or improvements here, so if you have ideas don't hesitate to complete the issue related to the prestafony project :)

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 15, 2018

@PierreRambaud does it need extra work from me?

public function load($resource, $type = null)
{
if (true === $this->isLoaded) {
throw new \RuntimeException('Do not add the "module" loader twice.');

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 15, 2018

Contributor

Can you use namespace please :)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 16, 2018

Contributor

I'll do the update tomorrow,

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Jun 25, 2018

I missed the ping on this conversation, yeah all the tabs are using the legacy system. Even the new controllers are related to a legacy entry.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 25, 2018

This is something I need to address, but not required to let our team work on MBO ;)

@marionf marionf self-assigned this Jun 25, 2018

@marionf

This comment has been minimized.

Contributor

marionf commented Jun 25, 2018

@mickaelandrieu

I am sorry but can't test your PR because of this issue on develop branch:

capture du 2018-06-21 16-18-30

@marionf marionf added QA ✔️ and removed waiting for QA labels Jun 25, 2018

@marionf marionf removed their assignment Jun 25, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit 6251e07 into PrestaShop:develop Jun 25, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Jun 25, 2018

Thank you @mickaelandrieu

@mickaelandrieu mickaelandrieu referenced this pull request Jun 26, 2018

Closed

Roadmap migration (1.7.5) #10

26 of 40 tasks complete

@PierreRambaud PierreRambaud deleted the mickaelandrieu:play-with-admin-controllers branch Jun 26, 2018

@mickaelandrieu mickaelandrieu referenced this pull request Sep 5, 2018

Closed

Symfony migration roadmap for 1.7.5 #10301

27 of 40 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment