[MGNLDATA-100] Make dependency on the cache module optional Created: 03/Apr/10 Updated: 16/Dec/13 Resolved: 16/Dec/13 |
|
| Status: | Closed |
| Project: | Magnolia Data Module (closed) |
| Component/s: | None |
| Affects Version/s: | 1.4.2 |
| Fix Version/s: | 1.5 |
| Type: | Improvement | Priority: | Major |
| Reporter: | Fabrizio Giustina | Assignee: | Fabrizio Giustina |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||||||
| Issue Links: |
|
||||||||
| Template: |
|
||||||||
| Acceptance criteria: |
Empty
|
||||||||
| Date of First Response: | |||||||||
| Comments |
| Comment by Fabrizio Giustina [ 03/Apr/10 ] |
|
added <optional> to the module descriptor and fixed the version handler in order to silently ignore the failure when instantiating info.magnolia.module.cache.RegisterWorkspaceForCacheFlushingTask |
| Comment by Magnolia International [ 04/Apr/10 ] |
|
There are IsModuleAvailable sort of tasks (can't recall the exact class name now); please revert this and use those instead. |
| Comment by Fabrizio Giustina [ 05/Apr/10 ] |
|
reworked patch using IsModuleInstalledOrRegistered task |
| Comment by Fabrizio Giustina [ 05/Apr/10 ] |
|
no way, we can't use the the IsModuleInstalledOrRegistered way: RegisterWorkspaceForCacheFlushingTask needs to be passed to the constructor so it will fail with a noclassdeffound exception if the media module is not available, far before the task can check if the module is registered (remember the RegisterWorkspaceForCacheFlushingTask is the cache module). Catching the exception is IMHO the safer way at this moment, other solutions would be:
so, rolling back to the catch() solution at the moment, it makes no sense that lots of modules depends on the cache module just because they need to use a task found in the cache module when the cache module is installed... |
| Comment by Magnolia International [ 06/Apr/10 ] |
|
at the very least then this should catch the specific exception and not everything else ... |
| Comment by Philipp Bärfuss [ 08/Apr/10 ] |
|
I propose something like: IsModuleInstalledOrRegistered(.., new AbstractTask(){ execute(ctx){ new RegisterWorkspaceForCacheFlushingTask().execute(ctx) } } This works as the instantiation will only happen on execution which will only be called if the module is installed. |
| Comment by Fabrizio Giustina [ 08/Apr/10 ] |
it's a good solution, the only concern is that the IsModuleInstalledOrRegistered executes if it just find the /modules/cache node on jcr... in case of the cache module not being installed but with a leftover configuration all the other moduleVersionHandlers will still die with a ClassNotFound exception. |
| Comment by Philipp Bärfuss [ 09/Apr/10 ] |
|
True, but I think this is an acceptable exception as removing just the jar is never enough. This would for instance also left the cache filter behind which can not be instantiated either. So unless the node /modules/<names> is removed a module is considered installed. |
| Comment by Jan Haderka [ 30/Sep/10 ] |
|
Previous changes to the code in single patch file. The changes have been rolled back due to issue not being closed at the time of release. |
| Comment by Jan Haderka [ 19/Nov/10 ] |
|
Fabrizio, could you have a look at this issue again and re-apply the patch and close the issue if it is done? |