Improve extendability of the new product page form #28752

jolelievre · 2022-06-14T18:08:57Z

Questions	Answers
Branch?	8.0.x
Description?	The new product page needs to be easily extendable by modules, so far we could already add some custom fields anywhere in the existing tabs but not add our own custom tab To allow this a new `NavigationTabType` was introduced it basically allows you to display its children in a tabbed navigation each child represents a Tab This component is used as a base for the product form Also in order to limit the twig content and especially the association of form theme a `FormThemeExtension` which introduces a new `form_theme` option that allows you to customize the form theme from the PHP form type code It's used on the whole product page which uses many sub form themes, but it will also allow modules to use their own form theme for their fields, or to customize the Core form theme if they need to Finally for backward compatibility a custom `ExtraModulesType` form type was introduced to handle the integration of the `displayAdminProductsExtra` hook used in modules
Type?	new feature
Category?	BO
BC breaks?	yes
Deprecations?	no
Fixed ticket?	Fix #28994
Related PRs	You should use this PR for the example module PrestaShop/example-modules#50
How to test?	See How to test section below
Possible impacts?	IDs may have changed on some elements which could break some JS behaviour because the selectors wouldn't match anymore, that's why it'd be good to check that all fields work fine but not in a extensive way ^^ One edit, save, it's updated it's ok!

How to test

This PR changes the structure of the form a bit, so we need to check that the form behaves like before and that all the fields work as expected The idea is not to check everything extensively but only a quick run on each input to make sure they are not ignored because of some naming mismatch just to be sure Simply edit the field, save, the field is saved, done!

Also to ensure the extendability of modules works fine, this is the actual purpose of this PR, you should use the example module demoproductform and install it using the branch of this PR. Once it is installed it should add three new custom things in the product page:

Custom field in description tab

Custom tab with a custom price field

Configuration panel in the Modules tab

It is nearly a resume of the custom field used as a POC to show the feature is still usable for backward compatibility with modules relying on the displayAdminProductsExtra hook.

Both fields should be editable and persisted in the DB, they are not used in FO right now only in the BO. If this PR is validated with the example module then it will also validate the module PR so we can merge it to provide an example for the community.

Finally, as all the PR for the product page V2 this PR MUST NOT be tested for multishop.

matks

Ok for the code. I provided some suggestions. Are you sure the form still renders like before? You cleaned a lot of twig files

matks · 2022-07-01T08:26:45Z

src/Adapter/Shop/Url/HelpProvider.php

+            $title = $this->translator->trans('Help', [], 'Admin.Global');
+        }
+
+        $docLink = urlencode('https://help.prestashop.com/' . $this->legacyContext->getEmployeeLanguageIso() . '/doc/'


Nice to have: no hardcoded URL domain?

Usage of http_build_query?

That could be an improvement indeed, I didn't look further and just copied the existing code from here

PrestaShop/src/PrestaShopBundle/Controller/Admin/FrameworkBundleAdminController.php

Lines 177 to 193 in d0f9ede

protected function generateSidebarLink($section, $title = false)

{

$version = $this->get('prestashop.core.foundation.version')->getSemVersion();

$legacyContext = $this->get('prestashop.adapter.legacy.context');

if (empty($title)) {

$title = $this->trans('Help', 'Admin.Global');

}

$docLink = urlencode('https://help.prestashop.com/' . $legacyContext->getEmployeeLanguageIso() . '/doc/'

. $section . '?version=' . $version . '&country=' . $legacyContext->getEmployeeLanguageIso());

return $this->generateUrl('admin_common_sidebar', [

'url' => $docLink,

'title' => $title,

]);

}

The purpose here was not to improve this code but put it in a reusable service. A refacto might be worth though and we could also refacto the generateSidebarLink so that it uses the new HelpProvider But I don't think it's in this PR's scope

src/PrestaShopBundle/Form/Admin/Sell/Product/ExtraModulesType.php

zuk3975 · 2022-07-18T07:19:26Z

src/Core/Grid/Definition/Factory/ProductGridDefinitionFactory.php

@@ -157,7 +157,7 @@ protected function getColumns()
                        'route' => 'admin_products_v2_edit',
                        'route_param_name' => 'productId',
                        'route_param_field' => 'id_product',
-                        'fragment' => 'tab-pricing-tab',
+                        'fragment' => 'tab-product_pricing-tab',


question not really related to this pr, but why is it called fragment? (this is a fragment of what?)

It was debated for a long time actually in this PR #22731 (comment) 😅

But TLDR; it's the naming used by Symfony so I didn't argue back in the days:
https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Routing/Generator/UrlGenerator.php#L297-L311

If you think it should be changed and have a better suggestion I'm open though, it's now or never 😅

I wouldn't have question if we use the "fragment" in router (or other URL related service). But its too generic here because its Grid column, it doesn't relate directly with URL context. its like when we introduced route_param_name option in same Column definition - we didin't call it param_name. So I would suggest renaming this option to somewhat more specific, like url_fragment? route_fragment wdyt?

zuk3975 · 2022-07-18T07:23:34Z

src/PrestaShopBundle/Form/Admin/Extension/FormThemeExtension.php

+ * form is automatic with a single form_row it is hard to define the form theme from twig that's one of the
+ * use cases of this extension.
+ *
+ * An addition option exist with this extension use_default_themes which is the equivalent of the only keyword


Suggested change

* An addition option exist with this extension use_default_themes which is the equivalent of the only keyword

* Additional option supported by this extension - "use_default_themes" is the equivalent of the "only" keyword

I rewrote the whole description a little with more detailed examples and a clearer explanation about the default theme from config. Let me know if it's enough or if I should detail some parts a bit more.

zuk3975 · 2022-07-18T07:30:08Z

src/PrestaShopBundle/Form/Admin/Extension/FormThemeExtension.php

+ * It is true by default, meaning to get the same behaviour as the twig keyword only you need to specify use_default_themes
+ * to false.


This sentence needs fixing (put commas where needed or so). Because by reading it, I can't understand which option means what 😄
Do I need to set it to false to "get the same behaviour as the twig keyword only" or `the true by default means the same behavior as the twig keyword "only"?

smth like: Default value is "true" and it means the twig is using default global theme and your custom theme. Set it to "false" if you don't want to use the global theme at all

zuk3975 · 2022-07-18T07:39:47Z

src/PrestaShopBundle/Form/Admin/Extension/FormThemeExtension.php

+                // Specify if default theme should be used as well
+                'use_default_themes' => true,
+            ])
+            ->setAllowedTypes('form_theme', ['null', 'string', 'array'])


when you pass an array of form themes it uses them all?

Yes, it stacks them, I added a new example to explain this.

zuk3975 · 2022-07-18T07:48:29Z

src/PrestaShopBundle/Form/Admin/Type/NavigationTabType.php

+        parent::buildForm($builder, $options);
+        if (!empty($options['toolbar_buttons'])) {
+            if ($builder->has('toolbar_buttons')) {
+                $this->logger->warning('You should not add a field which name is toolbar_buttons on this component as it is used internally.');


Suggested change

$this->logger->warning('You should not add a field which name is toolbar_buttons on this component as it is used internally.');

$this->logger->warning('You should not add an option "toolbar_buttons" in this component as such option is already used internally.');

No, actually I'm not talking about an option here, but really a form field.

In the following lines, I'm doing $builder->add('toolbar_buttons', ...) meaning the form.tollbar_buttons is now a new field which type is ButtonCollectionType I need it to render it in the twig theme thus relying on Form component and twig tools.

But the field name is hard-coded here, meaning if a developer already used it then it will be overridden, that's why I check if the field already existed. I hesitated with an exception but I thought maybe it's a bit brutal, but maybe it would actually be easier to understand for the developer. Thus a clear exception is breaking everything instead of a field that magically disappears.

An alternative could be to throw an exception in debug mode and log a warning in prod mode. What do you think?

that sounds like a good alternative 👍

also as we discussed, maybe adding an underscore like this _toolbar_buttons would reduce the risk of naming overlap

zuk3975

Couple comments (even though they are mostly about documentation wording, I think it must be adjusted, because it is hard to understand the meaning now. At least this one #28752 (comment))

jolelievre

Thanks for the feedback @zuk3975

I rebased the branch and improved the PHPDoc, I hope it's clearer now. Don't hesitate to tell me if there are still some complicated things to improve.

jolelievre · 2022-08-03T17:05:56Z

src/Core/Grid/Definition/Factory/ProductGridDefinitionFactory.php

@@ -157,7 +157,7 @@ protected function getColumns()
                        'route' => 'admin_products_v2_edit',
                        'route_param_name' => 'productId',
                        'route_param_field' => 'id_product',
-                        'fragment' => 'tab-pricing-tab',
+                        'fragment' => 'tab-product_pricing-tab',


It was debated for a long time actually in this PR #22731 (comment) 😅

But TLDR; it's the naming used by Symfony so I didn't argue back in the days:
https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Routing/Generator/UrlGenerator.php#L297-L311

If you think it should be changed and have a better suggestion I'm open though, it's now or never 😅

jolelievre · 2022-08-03T17:17:58Z

src/PrestaShopBundle/Form/Admin/Extension/FormThemeExtension.php

+ * form is automatic with a single form_row it is hard to define the form theme from twig that's one of the
+ * use cases of this extension.
+ *
+ * An addition option exist with this extension use_default_themes which is the equivalent of the only keyword


I rewrote the whole description a little with more detailed examples and a clearer explanation about the default theme from config. Let me know if it's enough or if I should detail some parts a bit more.

jolelievre · 2022-08-03T17:18:46Z

src/PrestaShopBundle/Form/Admin/Extension/FormThemeExtension.php

+                // Specify if default theme should be used as well
+                'use_default_themes' => true,
+            ])
+            ->setAllowedTypes('form_theme', ['null', 'string', 'array'])


Yes, it stacks them, I added a new example to explain this.

jolelievre · 2022-08-03T17:23:10Z

src/PrestaShopBundle/Form/Admin/Type/NavigationTabType.php

+        parent::buildForm($builder, $options);
+        if (!empty($options['toolbar_buttons'])) {
+            if ($builder->has('toolbar_buttons')) {
+                $this->logger->warning('You should not add a field which name is toolbar_buttons on this component as it is used internally.');


No, actually I'm not talking about an option here, but really a form field.

In the following lines, I'm doing $builder->add('toolbar_buttons', ...) meaning the form.tollbar_buttons is now a new field which type is ButtonCollectionType I need it to render it in the twig theme thus relying on Form component and twig tools.

But the field name is hard-coded here, meaning if a developer already used it then it will be overridden, that's why I check if the field already existed. I hesitated with an exception but I thought maybe it's a bit brutal, but maybe it would actually be easier to understand for the developer. Thus a clear exception is breaking everything instead of a field that magically disappears.

An alternative could be to throw an exception in debug mode and log a warning in prod mode. What do you think?

zuk3975 · 2022-08-04T08:59:10Z

src/PrestaShopBundle/Form/Admin/Sell/Product/ExtraModulesType.php

+ */
+class ExtraModulesType extends TranslatorAwareType
+{
+    public const HOOK_NAME = 'displayAdminProductsExtra';


Nothing to change here, just wonder if we should pursue to use constants everywhere where using hooks (and why haven't we tried to do so before? like if we would have some static class to hold all the hooks or so?) or it sounds bad?
There are so many "magic strings" hanging now (i mean the hook names) everywhere 🤷‍♂️

Yeah probably for the core hooks at least it could make sense to have some consts, but it's not the purpose of this PR to refacto everything 😅 And it won't be applicable everywhere because we also have many dynamic hooks that are built with placeholders such as the object class names
But it's an idea to look into for future versions I guess

matthieu-rolland

LGTM, minus the small improvement that could be made about potential naming overlap on toolbar_buttons

matthieu-rolland · 2022-08-04T09:28:55Z

src/PrestaShopBundle/Form/Admin/Type/NavigationTabType.php

+        parent::buildForm($builder, $options);
+        if (!empty($options['toolbar_buttons'])) {
+            if ($builder->has('toolbar_buttons')) {
+                $this->logger->warning('You should not add a field which name is toolbar_buttons on this component as it is used internally.');


that sounds like a good alternative 👍

also as we discussed, maybe adding an underscore like this _toolbar_buttons would reduce the risk of naming overlap

…ifferent form builder hook name

…tionTabType

matks

OK for me ✅

matks · 2022-08-08T09:21:33Z

tests/Integration/PrestaShopBundle/Controller/Sell/Catalog/ProductControllerTest.php

@@ -225,7 +225,7 @@ protected function getFilterSearchButtonSelector(): string
     */
    protected function getCreateSubmitButtonSelector(): string
    {
-        return 'product_create';
+        return 'create_product_create';


matks · 2022-08-08T09:29:08Z

src/PrestaShopBundle/Form/Toolbar/ToolbarButtonsProviderInterface.php

@@ -1,4 +1,5 @@
-{#**
+<?php


Funny GitHub believes this is a conversion from twig to php

khouloudbelguith

Hi @jolelievre,

Automatic tests => OK
I checked those tests:
Without module & With demo module installed:

Create a standard product V2 => OK
Create a product with combinations V2 => OK
Create a virtual product V2 => OK
Create a pack product V2 => OK

In the other case,

I tried to import products & edit them => All tabs are OK
I tried to add a product via API => OK
I tried to edit the product page with mobile => OK
I tried with RTL language => OK

But as discussed in slack, I have an exception when edit/add a product V2 when installing a custom module, I cannot publish it in public.

Without PR => ok

Thanks!

jolelievre · 2022-08-10T14:30:43Z

Hi @khouloudbelguith

I managed to reproduce the bug with the module you provided. However, I have the same error even using the 8.0.x branch but only when I use PHP8.0. It doesn't come from this PR but from the module's code which uses this code implode($id_langs, ', ') but in PHP 8.0 the order of the parameters for this function were reversed so it should be implode(', ', $id_langs)

But anyway the PR is not responsible for this bug

khouloudbelguith

Hi @jolelievre,

Thanks for your feedback.
As commented here: #28752 (review)

It is ok ✔️

Thanks!

prestonBot · 2022-08-10T15:44:55Z

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

matks · 2022-08-10T15:56:25Z

Thank you @jolelievre

prestonBot added Feature Type: New Feature BC break Type: Introduces a backwards-incompatible break labels Jun 14, 2022

jolelievre force-pushed the tab-form-type branch 3 times, most recently from dd0bbfc to 9454b70 Compare June 29, 2022 17:48

jolelievre marked this pull request as ready for review June 30, 2022 13:25

jolelievre requested a review from a team as a code owner June 30, 2022 13:25

prestonBot added the 8.0.x Branch label Jun 30, 2022

jolelievre changed the title ~~Introduce FormThemeExtension and NavigationTabType for product page form~~ Improve extendability of the new product page form Jun 30, 2022

matks added the migration symfony migration project label Jun 30, 2022

matks previously approved these changes Jul 1, 2022

View reviewed changes

kpodemski mentioned this pull request Jul 5, 2022

hook actionProductFormBuilderModifier not called #28955

Closed

2 tasks

jolelievre requested review from NeOMakinG, matthieu-rolland and zuk3975 July 6, 2022 16:44

jolelievre linked an issue Jul 7, 2022 that may be closed by this pull request

Extend the product page #28994

Closed

6 tasks

jolelievre dismissed matks’s stale review via 9fd976f July 12, 2022 12:44

jolelievre force-pushed the tab-form-type branch from 20b0708 to 9fd976f Compare July 12, 2022 12:44

zuk3975 reviewed Jul 18, 2022

View reviewed changes

zuk3975 requested changes Jul 18, 2022

View reviewed changes

jolelievre force-pushed the tab-form-type branch from 4badbf3 to f4187be Compare August 3, 2022 17:28

jolelievre commented Aug 3, 2022

View reviewed changes

zuk3975 reviewed Aug 4, 2022

View reviewed changes

matthieu-rolland reviewed Aug 4, 2022

View reviewed changes

jolelievre added 5 commits August 8, 2022 11:17

Rename LinkColumn option into route_fragment

793916e

Change create product form prefix so that it has different name and d…

ed1e79d

…ifferent form builder hook name

Ignore _token field in the tab creation

77508bc

Add option label_tab to force a label inside a tab content for Naviga…

a64eb9a

…tionTabType

Define specific SEO tab inside label

8efaeef

matks previously approved these changes Aug 8, 2022

View reviewed changes

Integrate new Documentation service in help provider

af39005

jolelievre dismissed stale reviews from matks and zuk3975 via af39005 August 8, 2022 09:35

jolelievre force-pushed the tab-form-type branch from d0e3ce1 to af39005 Compare August 8, 2022 09:35

zuk3975 approved these changes Aug 8, 2022

View reviewed changes

matks approved these changes Aug 8, 2022

View reviewed changes

matthieu-rolland approved these changes Aug 8, 2022

View reviewed changes

khouloudbelguith reviewed Aug 8, 2022

View reviewed changes

khouloudbelguith added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Aug 8, 2022

khouloudbelguith approved these changes Aug 10, 2022

View reviewed changes

khouloudbelguith added QA ✔️ Status: check done, code approved and removed Waiting for author Status: action required, waiting for author feedback labels Aug 10, 2022

khouloudbelguith added this to the 8.0.0 milestone Aug 10, 2022

matks merged commit 435dc79 into PrestaShop:8.0.x Aug 10, 2022

matks mentioned this pull request Aug 10, 2022

Extend the product page #28994

Closed

6 tasks

jolelievre deleted the tab-form-type branch October 17, 2022 17:38

kpodemski mentioned this pull request Jan 26, 2023

PrestaShop 8.1 - list of items to be documented [wip] PrestaShop/docs#1602

Closed

28 tasks

kpodemski mentioned this pull request Feb 28, 2023

Extendibility of the new product page PrestaShop/docs#1620

Closed

kpodemski added Documentation ✔️ Developer documentation is up-to-date and removed Needs documentation Needs an update of the developer documentation labels Jun 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve extendability of the new product page form #28752

Improve extendability of the new product page form #28752

jolelievre commented Jun 14, 2022 •

edited

Loading

matks left a comment

matks Jul 1, 2022

matks Jul 1, 2022

jolelievre Jul 1, 2022

zuk3975 Jul 18, 2022

jolelievre Aug 3, 2022

zuk3975 Aug 4, 2022

zuk3975 Jul 18, 2022 •

edited

Loading

jolelievre Aug 3, 2022

zuk3975 Jul 18, 2022

zuk3975 Jul 18, 2022 •

edited

Loading

zuk3975 Jul 18, 2022 •

edited

Loading

jolelievre Aug 3, 2022

zuk3975 Jul 18, 2022 •

edited

Loading

jolelievre Aug 3, 2022

matthieu-rolland Aug 4, 2022 •

edited

Loading

zuk3975 left a comment •

edited

Loading

jolelievre left a comment

jolelievre Aug 3, 2022

jolelievre Aug 3, 2022

jolelievre Aug 3, 2022

jolelievre Aug 3, 2022

zuk3975 Aug 4, 2022

jolelievre Aug 4, 2022

matthieu-rolland left a comment

matthieu-rolland Aug 4, 2022 •

edited

Loading

matks left a comment

matks Aug 8, 2022

matks Aug 8, 2022

khouloudbelguith left a comment

jolelievre commented Aug 10, 2022

khouloudbelguith left a comment

prestonBot commented Aug 10, 2022

matks commented Aug 10, 2022

	protected function generateSidebarLink($section, $title = false)
	{
	$version = $this->get('prestashop.core.foundation.version')->getSemVersion();
	$legacyContext = $this->get('prestashop.adapter.legacy.context');

	if (empty($title)) {
	$title = $this->trans('Help', 'Admin.Global');
	}

	$docLink = urlencode('https://help.prestashop.com/' . $legacyContext->getEmployeeLanguageIso() . '/doc/'
	. $section . '?version=' . $version . '&country=' . $legacyContext->getEmployeeLanguageIso());

	return $this->generateUrl('admin_common_sidebar', [
	'url' => $docLink,
	'title' => $title,
	]);
	}

	* An addition option exist with this extension use_default_themes which is the equivalent of the only keyword
	* Additional option supported by this extension - "use_default_themes" is the equivalent of the "only" keyword

		* It is true by default, meaning to get the same behaviour as the twig keyword only you need to specify use_default_themes
		* to false.

	$this->logger->warning('You should not add a field which name is toolbar_buttons on this component as it is used internally.');
	$this->logger->warning('You should not add an option "toolbar_buttons" in this component as such option is already used internally.');

		@@ -1,4 +1,5 @@
		{#**
		<?php

Improve extendability of the new product page form #28752

Improve extendability of the new product page form #28752

Conversation

jolelievre commented Jun 14, 2022 • edited Loading

How to test

Custom field in description tab

Custom tab with a custom price field

Configuration panel in the Modules tab

Finally, as all the PR for the product page V2 this PR MUST NOT be tested for multishop.

matks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuk3975 Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuk3975 Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

zuk3975 Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuk3975 Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthieu-rolland Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

zuk3975 left a comment • edited Loading

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthieu-rolland left a comment

Choose a reason for hiding this comment

matthieu-rolland Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

matks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khouloudbelguith left a comment

Choose a reason for hiding this comment

jolelievre commented Aug 10, 2022

khouloudbelguith left a comment

Choose a reason for hiding this comment

prestonBot commented Aug 10, 2022

matks commented Aug 10, 2022

jolelievre commented Jun 14, 2022 •

edited

Loading

zuk3975 Jul 18, 2022 •

edited

Loading

zuk3975 Jul 18, 2022 •

edited

Loading

zuk3975 Jul 18, 2022 •

edited

Loading

zuk3975 Jul 18, 2022 •

edited

Loading

matthieu-rolland Aug 4, 2022 •

edited

Loading

zuk3975 left a comment •

edited

Loading

matthieu-rolland Aug 4, 2022 •

edited

Loading