[MGNLRES-42] Issues with InstallResourcesTask and InstallResourceTask Created: 08/Mar/12  Updated: 29/Mar/22  Resolved: 20/Dec/13

Status: Closed
Project: Magnolia Resources Module
Component/s: management
Affects Version/s: 1.5, 2.0.1
Fix Version/s: 2.2.1

Type: Improvement Priority: Major
Reporter: Magnolia International Assignee: Roman Kovařík
Resolution: Fixed Votes: 0
Labels: maintenance
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to MGNLRES-89 Create constants for resource types Closed
causality
is causing MGNLRES-103 InstallTextResourcesTask's pattern is... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:
Team: Nucleus

 Description   

The 1.5 release of the resources module introduced two things

  • support for binaries
  • new templates (i.e same changes as for M4.5, STK2.0 etc)

Following those changes, I have a couple of issues with InstallResourcesTask and InstallResourceTask

  1. Why do I need to know the "template" used for binaries ?
  2. There is a whole bunch of code branching off in InstallResourceTask if binary is true
    It seems to me that the template parameter is meaningless when binary is true. At the very least, the defaults should be known by the Task, and the user should not have to know about it. Instead of adding a boolean flag to every variation of InstallResourceTask constructors:
    • Just have a different Task class altogether for binary resource, OR
    • Have static factory methods, with meaningful names and meaningful parameters.
  3. InstallResourcesTask.determineTemplate is wrong. Has per it's javadoc: "If no template has been defined the file extension is used.". That was valid up until 1.4, since the templates were called js and css respectively.
    • The method needs to be updated to reflect new template IDs
    • The method might be just as relevant in InstallResourceTask instead (i.e no need to know the template name when installing a single resource either) - move it ! (moved to ResourceTypes - MGNLRES-89)
    • The method is irrelevant for binaries (same template name for all kinds of binaries)

In fact, if the module comes with predefined templates for css, js, binaries, and nothing else, it might even be a good idea to use static factory methods as suggested above, with explicit method names for each kind of resource we want to support, instead of relying on a file's extension ?



 Comments   
Comment by Magnolia International [ 08/Mar/12 ]

As a side note, it might also be judicious to check/improve the Javadoc for InstallReferenceResourceTask. I have no idea what that is for.

Comment by Jan Haderka [ 04/Dec/13 ]

Please squeeze all those commits in one before merging.

Comment by Roman Kovařík [ 20/Dec/13 ]

InstallBinaryResourceTaskTest fails on mac.

Comment by Mikaël Geljić [ 20/Dec/13 ]

see additional QAs:

https://git.magnolia-cms.com/gitweb/?p=modules/resources.git;a=commit;h=d6f7423a096b0d77d23574339e3d5c910f711788
https://git.magnolia-cms.com/gitweb/?p=modules/resources.git;a=commit;h=9356e05ed71076eb846ac6b04943d9c31ab75a6a

Generated at Mon Feb 12 06:46:48 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.