Prestashop: [1.7.7-beta.1] [BO] order of js loaded from module has been modified

Created on 20 May 2020  路  16Comments  路  Source: PrestaShop/PrestaShop

Describe the bug

@Quetzacoalt91 found out that the order of JS on the back office has been modified and can break some jquery migration compatibility scripts of a module.

This is in the configuration page of the module in the BO.

1.7.7 order ->
```


1.7.6 order ->

```<script type="text/javascript" src="/js/jquery/jquery-1.11.0.min.js"></script>
<script type="text/javascript" src="/js/jquery/jquery-1.11.0.min.js"></script>
<script type="text/javascript" src="/js/jquery/jquery-migrate-1.2.1.min.js"></script>
<script type="text/javascript" src="/modules/seoexpert/views/js/jquery-2.1.0.min.js"></script>
<script type="text/javascript" src="/modules/seoexpert/views/js/mynoConflict.js"></script>
<script type="text/javascript" src="/modules/seoexpert/views/js/bootstrap-select.min.js"></script>
<script type="text/javascript" src="/modules/seoexpert/views/js/bootstrap-dialog.js"></script>
<script type="text/javascript" src="/modules/seoexpert/views/js/jquery.autosize.min.js"></script>
<script type="text/javascript" src="/modules/seoexpert/views/js/jquery.dataTables.js"></script>
<script type="text/javascript" src="/modules/seoexpert/views/js/jquery.smartWizard.js"></script>
<script type="text/javascript" src="/modules/seoexpert/views/js/DT_bootstrap.js"></script>
<script type="text/javascript" src="/modules/seoexpert/views/js/dynamic_table_init.js"></script>
<script type="text/javascript" src="/modules/seoexpert/views/js/jstree.min.js"></script>
<script type="text/javascript" src="/modules/seoexpert/views/js/faq.js"></script>
<script type="text/javascript" src="/modules/seoexpert/views/js/seoexpert.js"></script>

Expected behavior

Keep same order, i guess 馃

Additional information

  • PrestaShop version: 1.7.7-beta.1
  • PHP version: 7.3.17
1.7.7.0 BO Bug CO Fixed Major Order PR available

All 16 comments

Thanks for opening this issue! We will help you to keep its state consistent

On previous versions of PS 1.7, even if jQuery was always loaded, having the method addJquery allowed us to order the load of JS files at our convenience.

We saw an impact on modules having their own jQuery version coupled with noConflict. The objective is to make sure they load a jQuery version compatible with their additional libraries and leave the core one untouched.

Now that method is not doing anything from 1.7.7, the list of JS files to load depends on the moment you call addJs() in the module. We could move these calls later in the module call, but all modules could still work as before if Controller::addJquery() was still adding the files in the list, even if it is deprecated.

@Quetzacoalt91 As addJquery do nothing more than throw an error, you can call this method where you want, it will change nothing at all.

@Quetzacoalt91 FYI am working on porting addCss() and addJS() functions for Symfony controllers in https://github.com/PrestaShop/PrestaShop/pull/19277

As for addJQuery(), the phpDoc says

     * @deprecated 1.7.7 jQuery is always included, this method should no longer be used

So if we use addJQuery() to attempt to make the 2 lists identical (1.7.6 and 1.7.7) we will have jQuery included twice ?

@Quetzacoalt91 As addJquery do nothing more than throw an error, you can call this method where you want, it will change nothing at all.

And this is because of this "nothing" we encounter issues. Previously, even if jQuery would be anyway included, we could chose its position in the assets list. This would allow modules to order the list, so they can add their own scripts relying on it.

So if we use addJQuery() to attempt to make the 2 lists identical (1.7.6 and 1.7.7) we will have jQuery included twice ?

I should be included only one time. PrestaShop filters redundant assets.

https://github.com/PrestaShop/PrestaShop/blob/3112ecde59959ab42fa6ee9ea23fcdd9278258c2/classes/controller/AdminController.php#L2346

@matks @Quetzacoalt91 should we put it in 177 kanban ?

@matks @Quetzacoalt91 should we put it in 177 kanban ?

I think yes, as it is an unwanted change of behavior that causes issues to modules

Yes please, there are potential side effects on modules relying on addJquery() on PS 1.7.7

@Quetzacoalt91 As addJquery do nothing more than throw an error, you can call this method where you want, it will change nothing at all.

And this is because of this "nothing" we encounter issues. Previously, even if jQuery would be anyway included, we could chose its position in the assets list. This would allow modules to order the list, so they can add their own scripts relying on it.

So there are 3 topics, if I understand correctly:

jQuery files are required in a new Order in 1.7.7

As some modules rely on this order, this creates issues.

Solution: make sure jQuery files are required like before, which means at the beginning of the <script type="text/javascript" src=...> list is that right ?

addJQuery() cannot be used anymore

Is that really an issue if previous bug is fixed ? Is there acceptable replacements for addJQuery() legacy function ?

jQuery has been upgraded

We have not mentioned it in this thread but we also upgrade jQuery version, mainly for security reasons. In theory we have preserved backward compatibility with a polyfill. Did you notice any issue though ?

Investigation work needed to know what has happened and see how to fix it.

Investigation work needed to know what has happened and see how to fix it.

Oh, we know what happened 馃槃 in 1.7.7 @matthieu-rolland did the huge work of reworking / upgrading jQuery in BO (and maybe FO too ?). This is one of the consequences of his work (which was needed and was hard nonetheless)

So if I get it right the problem is that before we could alter the position of jquery in the scripts list? Wouldn't it be much more simple just to put all the scripts from modules _after_ the ones from the core?

So if I get it right the problem is that before we could alter the position of jquery in the scripts list?

Exactly. Note it can be moved "earlier" in the list, but never towards the end.

Wouldn't it be much more simple just to put all the scripts from modules after the ones from the core?

That's what we did in the related module (SEOExpert). However I would not consider that modifying all the modules is a simple solution.

We still find in the documentation that JS and CSS files are added early, for instance with the hook displayHeader in https://devdocs.prestashop.com/1.7/modules/creation/displaying-content-in-front-office/

So there are 3 topics, if I understand correctly:

jQuery files are required in a new Order in 1.7.7

As some modules rely on this order, this creates issues.

Solution: make sure jQuery files are required like before, which means at the beginning of the <script type="text/javascript" src=...> list is that right ?

I would leave it as it is now on 1.7.7.

addJQuery() cannot be used anymore

Is that really an issue if previous bug is fixed ? Is there acceptable replacements for addJQuery() legacy function ?

I can have a look for you, keeping the previous behavior of addJquery might help even if it is deprecated.

jQuery has been upgraded

We have not mentioned it in this thread but we also upgrade jQuery version, mainly for security reasons. In theory we have preserved backward compatibility with a polyfill. Did you notice any issue though ?

Yes, we did. Some deprecated stuff has been removed, and we already updated our modules to stop rely on these attributes and methods. In my opinion, there was nothing to change on the core.

However, even if you added a polyfill for the $.live() method, we encountered issues we had to fix by replacing this method with $.on()

@zalexki Hey, could you show me the block where your js files are loaded (or even the whole block :) )? I want to compare something :)

Fixed by #19712

Was this page helpful?
0 / 5 - 0 ratings

Related issues

centoasa picture centoasa  路  3Comments

Prestaworks picture Prestaworks  路  3Comments

zuk3975 picture zuk3975  路  3Comments

matks picture matks  路  3Comments

vincent-dp picture vincent-dp  路  3Comments