deployphp / deployer

The PHP deployment tool with support for popular frameworks out of the box
https://deployer.org
MIT License
10.63k stars 1.49k forks source link

Magento 2: no assets generated when using custom magento_themes without split_static_deployment #3786

Closed enr-beck-mellow closed 6 months ago

enr-beck-mellow commented 8 months ago

When using a custom magento_themes configuration like ->set('magento_themes', [ 'myvendor/theme-at' => 'de_AT', 'myvendor/theme-be' => 'fr_BE nl_BE', ]); and not setting "split_static_deployment" to true no assets are generated.

The problem is in https://github.com/deployphp/deployer/blob/ef0087d988edd1a5a25339842d752aa1f452a923/recipe/magento2.php#L188-L193 where its only checked if there are multiple themes to process and then the language configurations from the custom configuration get passed as theme names.

peterjaap commented 8 months ago

Where do you get this notation from?

   ->set('magento_themes', [ 'myvendor/theme-at' => 'de_AT', 'myvendor/theme-be' => 'fr_BE nl_BE', ]);

I've never seen that before and it looks like we've just never supported it.

enr-beck-mellow commented 8 months ago

I found this in the documentation and in the magento2 recipe itself https://github.com/deployphp/deployer/blob/ef0087d988edd1a5a25339842d752aa1f452a923/recipe/magento2.php#L37-L41

peterjaap commented 8 months ago

Ah, that was introduced through https://github.com/deployphp/deployer/pull/3326 by @renttek. Julian could you take a look here?

renttek commented 8 months ago

I'll take a look into this

akosglue commented 6 months ago

I could reproduce this and I suggest the following change:

this foreach:

foreach (get('magento_themes') as $theme) {
    $themesToCompile .= ' -t ' . $theme;
}

should be changed to this:

$themes = array_is_list(get('magento_themes')) ? get('magento_themes') : array_keys(get('magento_themes'));
foreach ($themes as $theme) {
    $themesToCompile .= ' -t ' . $theme;
}

because it should obviously work for each of the following configurations:

//1.
set('magento_themes', []);

//2.
set('magento_themes', ['Magento/luma', 'Custom/another']);

//3.
set('magento_themes', [
    'Magento/blank'   => 'nl_BE',
    'Magento/luma'   => null,
    'myvendor/theme-at' => 'de_AT',
    'myvendor/theme-be' => 'fr_BE nl_BE',
    'Custom/another' => '{{static_content_locales}} it_IT',
]);

So either the array values are iterated or the keys of the associative array. (when it is empty the foreach is not reached because of the count check)

wdyt?

akosglue commented 6 months ago

and to be consistent with the other 2 method calls of setup:static-content:deploy, the -f switch should be added as well.

peterjaap commented 6 months ago

@akosglue that looks like a very good solution :+1: Could you PR it up?

akosglue commented 6 months ago

So I opened that PR but it fails when checking the docs. I have updated it though but now I am not sure hot to proceed with the fix. The changes are similar somehow, there it was ok, but in my PR it fails: https://github.com/deployphp/deployer/pull/3812/files https://github.com/deployphp/deployer/pull/3818/files

peterjaap commented 6 months ago

Merged by now