Skip to content
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

[POC] Support of modern controllers on module tabs #13889

Closed
wants to merge 1 commit into from
Closed

[POC] Support of modern controllers on module tabs #13889

wants to merge 1 commit into from

Conversation

mickaelandrieu
Copy link
Contributor

@mickaelandrieu mickaelandrieu commented May 21, 2019

Questions Answers
Branch? develop
Description? Allow modern controllers to be registered using Module::$tabs property.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? dunno
How to test? it's a POC, I won't have time to finish it (and unregistration may be updated accordingly?)

ping @Quetzacoalt91 for review ;)

Use

   $this->tabs = array(
        array(
            'name' => 'Yolo',
            'class_name' => 'adminmessageoftheday', // legacy
            'visible' => true,
            'parent_class_name' => 'AdminTraining',
        ),
        array(
            'name' => 'Yolo2',
            'class_name' => ExempleController::class,
            'legacy_class_name' => 'AdminTrainingIndexClass',
            'visible' => true,
            'parent_class_name' => 'AdminTraining',
        ));
admin_ps_training_index:
  path: /training/
  methods: [GET]
  defaults:
    _controller: 'PrestaShop\Training\Controller\Admin\ExempleController::indexAction'
    _legacy_controller: 'AdminTrainingIndexClass'
    _legacy_link: 'AdminTrainingIndexClass'

This change is Reviewable

@mickaelandrieu mickaelandrieu requested a review from a team as a code owner May 21, 2019 08:07
@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels May 21, 2019
@mickaelandrieu mickaelandrieu changed the title [POC] Support of SF controllers on module tabs [POC] Support of modern controllers on module tabs May 21, 2019
*
* @ToDo
*/
// Legacy Tab, to be replaced with Doctrine entity when right management
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe this comment/todo can be dropped now?


$className = $tabDetails->get('class_name');

if (is_subclass_of($className, FrameworkBundleAdminController::class)) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between this and

         if ($className instanceof FrameworkBundleAdminController) { 

Copy link
Contributor

Choose a reason for hiding this comment

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

An example si better than a long story: https://www.php.net/manual/en/internals2.opcodes.instanceof.php#109108
But don't know why you use is_subclass_of here 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I know the difference, my question was actually the same as yours. Is there a specific need here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I remember well, instanceof only accepts instances and not class names: if I'm wrong, I'm also in favor of using instanceof instead.

instanceof est utilisé pour déterminer si une variable PHP est un objet instancié d'une certaine classe

$className = $tabDetails->get('class_name');

if (is_subclass_of($className, FrameworkBundleAdminController::class)) {
$className = $tabDetails->get('legacy_class_name');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thinking about it, it's a very naive implementation: I should check for the existence of this parameter and throw an exception with a meaningful message pointing to the docs: wdyt?

@mickaelandrieu
Copy link
Contributor Author

Closed in favor of #12640

@mickaelandrieu mickaelandrieu deleted the sf-controllers-on-tabs branch July 4, 2019 10:18
@mickaelandrieu
Copy link
Contributor Author

mickaelandrieu commented Aug 21, 2019

ping @jolelievre : as this issue is still not fixed, would you consider this contribution ?

Just noticed you have worked on #14679 👍

@mickaelandrieu mickaelandrieu restored the sf-controllers-on-tabs branch August 21, 2019 08:14
@mickaelandrieu mickaelandrieu deleted the sf-controllers-on-tabs branch August 21, 2019 08:41
@jolelievre
Copy link
Contributor

Yes, I was about to tell you but you found it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Branch Improvement Type: Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants