Module upgrade bug #3378

Closed
craigh opened this Issue Jan 10, 2017 · 5 comments

Projects

None yet

3 participants

@craigh
Member
craigh commented Jan 10, 2017 edited
Q A
Zikula Version 2.0.0-alpha1
PHP Version 5.6.x

Expected behavior

proper redirect after module upgrade

Actual behavior

exception thrown

Steps to reproduce

here’s the bug I’ve discovered:

I’m using Core-2.0 but I think it would be the same in 1.4

  • start at home page
  • if you’re up to date then your DB should reflect that in modules table the extensions module is version 3.7.14
  • ensure zikula_asset_manager.combine: true in custom_parameters.yml
  • in the DB, manually change the version of extension module to 3.7.13 (state remains 3) (edited)
  • this would force a module upgrade to 3.7.14 (edited)
  • before doing anything further inspect appProdDebugProjectContainerUrlGenerator.php in cache folder
  • find en__RG__zikulablocksmodule_config_config
  • scroll across to route path info - should be 1 => '/blocks/config/config'
  • now visit extensions/module/list or use sidebar menu to click on extensions
  • reinspect appProdDebugProjectContainerUrlGenerator.php in cache folder
  • find zikulablocksmodule_config_config again
  • scroll to right, find route path info - is now 1 => '/config/config'
  • this cache file has been created after the page finished loading because all links on the current page work properly
  • e.g. using the sidebar menu to navigate to Blocks -> Settings produces /blocks/config/config as the link
  • but if you click the upgrade link for the extensions module
  • you will get an exception because it tries to find module/postmodify/%5B1%5D instead of extensions/module/postmodify/%5B1%5D (may be a different route in 1.4.x - maybe /module/view and should be extensions/module/view)

I’ve not yet tried this with any other modules. This may be specific to the ExtensionsModule or only Core modules, or it could affect all modules.

If you disable asset combination it works fine. but I cannot figure out why they are related.

please attempt to confirm/duplicate this behavior

@craigh craigh added this to the 1.4.6 milestone Jan 10, 2017
@craigh
Member
craigh commented Jan 10, 2017

The module is upgraded successfully, it is only the redirect that is broken.

If you surf to the homepage and then re-navigate to other portions of the site, the cache is again rebuilt and it works properly.

@Guite
Member
Guite commented Jan 11, 2017

I wonder if this somehow related to RouteLoader#prependBundlePrefix

cc @cmfcmf

@cmfcmf
Member
cmfcmf commented Jan 11, 2017 edited

I wonder if this somehow related to RouteLoader#prependBundlePrefix

cc @cmfcmf

That would've been my guess as well. Specifically: https://github.com/zikula/core/blob/1.4/src/system/RoutesModule/Routing/RouteLoader.php#L313-L319

@craigh
Member
craigh commented Jan 11, 2017

https://github.com/zikula/core/blob/4b12eb33d326cb6df239ac798586a2129e454d9e/src/system/RoutesModule/Translation/ZikulaPatternGenerationStrategy.php#L95-L95

is where the module url prefix is supposed to be added. The phpdoc notes for the method you both referenced say

 * We have to prepend the bundle prefix if
 * - routes are _not_ currently extracted via the command line and
 * - the route has i18n set to false.
 * This is because when extracting the routes, a bundle author only wants to translate the bare route
 * patterns, without a redundant and potentially customized bundle prefix in front of them.
 * If i18n is set to true, Zikula's customized pattern generation strategy will take care of it.
 * See Zikula\RoutesModule\Translation\ZikulaPatternGenerationStrategy

So, ZikulaPatternGenerationStrategy is supposed to take care of it. And indeed, it does run - just after it should - and therefore nothing gets committed to the cache file... still quite unsure what is going on here.

@craigh
Member
craigh commented Jan 12, 2017

I also failed to mention that this only happens when a module is available as an upgrade. If the module scan/sync operation turns up no available upgrades, then the route cache is not damaged.

@craigh craigh added a commit that closed this issue Feb 1, 2017
@craigh craigh fixes #3378 38dd742
@craigh craigh closed this in 38dd742 Feb 1, 2017
@craigh craigh added a commit that referenced this issue Feb 1, 2017
@craigh craigh changelog refs #3378 cb8e7fc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment