[7629] Update old ICE macros to support XB#775
[7629] Update old ICE macros to support XB#775jvega190 wants to merge 1 commit intocraftercms:developfrom
Conversation
|
This PR goes with craftercms/studio-ui#4335 |
WalkthroughThis change updates a Crafter CMS template file to migrate legacy Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/webapp/WEB-INF/templates/system/common/cstudio-support.ftl`:
- Around line 5-8: The current warning is shown for every iceAttr because the
template always emits data-craftercms-deprecated-ice-group even when iceGroup is
empty; change the template so the data-craftercms-deprecated-ice-group attribute
is only output when iceGroup is actually provided/non-empty, and update the
runtime check (deprecatedIceGroupCount / selector) to rely on that conditional
attribute (or check the attribute's value) so console.warn only runs for
elements where iceGroup was explicitly set.
- Around line 83-85: Wrap the inline deprecation console.warn scripts so they
only run in preview/authoring mode by guarding them with the existing
modePreview check (e.g. only emit or execute the script when modePreview is
truthy); locate the offending script that logs "Non-working macro 'ice'" and
change it to run inside that modePreview conditional, and apply the same change
to the other occurrences (the similar inline scripts at the other two locations
referenced).
- Around line 44-53: The $model is assigned too early using the elvis default
(component!contentModel) which yields an empty hash when component is omitted;
remove the top-level "<#local $model = component!contentModel>" and instead set
$model inside the existing conditional: in the branch where
"component?has_content" assign "$model = component.contentModel" and
"$collection = component[target]!{}"; in the else branch assign "$model =
contentModel" and "$collection = contentModel[target]!{}" so the macro
(componentContainerAttr) mirrors the pattern used by componentAttr/iceAttr and
avoids the empty-hash fallback causing __PATH_NOT_FOUND__/__ID_NOT_FOUND__ to be
emitted.
- Line 2: The template calls <@initExperienceBuilder props=xbProps /> without
ensuring xbProps exists; update the deprecated macros (toolSupport and
cstudioOverlaySupport) to defensively handle an absent xbProps by either adding
an optional macro parameter named xbProps with a safe default or by checking if
xbProps is defined before calling initExperienceBuilder and supplying a fallback
(empty map/object) when missing; ensure the change targets the call site that
invokes initExperienceBuilder so any external callers of these deprecated macros
won’t error if they don’t pass xbProps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5bf032b5-a02f-4025-a99b-1d663096551a
📒 Files selected for processing (1)
src/main/webapp/WEB-INF/templates/system/common/cstudio-support.ftl
| <#if modePreview> | ||
| <script src="/studio/static-assets/libs/requirejs/require.js" data-main="/studio/overlayhook?.js"></script> | ||
| </#if> | ||
| <@initExperienceBuilder props=xbProps /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect xbProps declarations/usages and toolSupport call sites.
# Expected: Either xbProps is always assigned before toolSupport/cstudioOverlaySupport runs,
# or the macro should use a default fallback as suggested.
rg -n -C3 --pcre2 '(<#(assign|local|global)\s+xbProps\b|\bxbProps\b|<@(?:toolSupport|cstudioOverlaySupport)\b)'Repository: craftercms/engine
Length of output: 2455
🏁 Script executed:
#!/bin/bash
# Find all uses of toolSupport and cstudioOverlaySupport macros
rg -n '<@(?:toolSupport|cstudioOverlaySupport)\b' --type ftlRepository: craftercms/engine
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check if xbProps is typically defined at a higher scope or in parent templates
rg -n 'xbProps' --type ftlRepository: craftercms/engine
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Look at the context where toolSupport is used - examine calling templates
rg -B5 -A2 '<@toolSupport\b' --type ftlRepository: craftercms/engine
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Find all uses of toolSupport and cstudioOverlaySupport macros in .ftl files
rg -n '<@(?:toolSupport|cstudioOverlaySupport)\b' '\.ftl$'Repository: craftercms/engine
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Check xbProps usage across all .ftl files
rg -n 'xbProps' '\.ftl$'Repository: craftercms/engine
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Look at templates that include or call cstudio-support.ftl
rg -n 'cstudio-support' '\.ftl$'Repository: craftercms/engine
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Find all uses of toolSupport and cstudioOverlaySupport macros in .ftl files
rg -n '<@(?:toolSupport|cstudioOverlaySupport)\b' -g '*.ftl'Repository: craftercms/engine
Length of output: 149
🏁 Script executed:
#!/bin/bash
# Check xbProps usage across all .ftl files
rg -n 'xbProps' -g '*.ftl'Repository: craftercms/engine
Length of output: 408
🏁 Script executed:
#!/bin/bash
# Look at templates that include or reference cstudio-support
rg -n 'cstudio-support' -g '*.ftl'Repository: craftercms/engine
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Find all uses of cstudioOverlaySupport macro
rg -n 'cstudioOverlaySupport' -g '*.ftl'Repository: craftercms/engine
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Check if these macros are referenced in Java or other source files
rg -n '(?:toolSupport|cstudioOverlaySupport)' --type javaRepository: craftercms/engine
Length of output: 43
🏁 Script executed:
#!/bin/bash
# List the structure of cstudio-support.ftl to understand the full context
cat -n src/main/webapp/WEB-INF/templates/system/common/cstudio-support.ftl | head -30Repository: craftercms/engine
Length of output: 1596
🏁 Script executed:
#!/bin/bash
# Find all includes of cstudio-support.ftl
rg -n 'include.*cstudio-support' -g '*.ftl'Repository: craftercms/engine
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for where cstudioOverlaySupport might be called
rg -n 'cstudioOverlaySupport' --type-listRepository: craftercms/engine
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Look for any FreeMarker includes/imports that might reference these macros
find . -name '*.ftl' -type f | head -20Repository: craftercms/engine
Length of output: 1315
🏁 Script executed:
#!/bin/bash
# Check if there are any other references to these macros in HTML or JSP files
rg -n '(?:toolSupport|cstudioOverlaySupport)' -g '*.html' -g '*.jsp'Repository: craftercms/engine
Length of output: 209
🏁 Script executed:
#!/bin/bash
# Check crafter.ftl to see its includes and macro definitions
cat -n src/main/webapp/WEB-INF/templates/system/common/crafter.ftlRepository: craftercms/engine
Length of output: 1161
🏁 Script executed:
#!/bin/bash
# Search for Java code that loads templates or includes
rg -n 'cstudio-support|toolSupport|cstudioOverlaySupport' --type javaRepository: craftercms/engine
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check for any includes of crafter.ftl or body_bottom
rg -n 'body_bottom|crafter' -g '*.ftl'Repository: craftercms/engine
Length of output: 6442
🏁 Script executed:
#!/bin/bash
# Search for any Java code that might include or reference cstudio-support.ftl
rg -n 'cstudio-support|toolSupport|cstudioOverlaySupport' --type java -g '*.java'Repository: craftercms/engine
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if there are any template configuration files that might load cstudio-support
find . -name '*.xml' -o -name '*.properties' -o -name '*.yaml' -o -name '*.yml' | xargs grep -l 'cstudio-support' 2>/dev/null || echo "No matches found"Repository: craftercms/engine
Length of output: 77
🏁 Script executed:
#!/bin/bash
# Check the git history to understand context - was this recently added or is it legacy?
git log --oneline -n 20 -- src/main/webapp/WEB-INF/templates/system/common/cstudio-support.ftlRepository: craftercms/engine
Length of output: 110
Guard optional xbProps before invoking Experience Builder in deprecated macros.
Line 2 references xbProps without defining it as a macro parameter or providing a fallback default. Although toolSupport and cstudioOverlaySupport appear to be unused legacy code (not called from any templates or Java code), applying a defensive default is appropriate if these deprecated macros are ever invoked from external code.
Proposed fix
- <@initExperienceBuilder props=xbProps />
+ <@initExperienceBuilder props=(xbProps!"{}") />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <@initExperienceBuilder props=xbProps /> | |
| <@initExperienceBuilder props=(xbProps!"{}") /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/webapp/WEB-INF/templates/system/common/cstudio-support.ftl` at line
2, The template calls <@initExperienceBuilder props=xbProps /> without ensuring
xbProps exists; update the deprecated macros (toolSupport and
cstudioOverlaySupport) to defensively handle an absent xbProps by either adding
an optional macro parameter named xbProps with a safe default or by checking if
xbProps is defined before calling initExperienceBuilder and supplying a fallback
(empty map/object) when missing; ensure the change targets the call site that
invokes initExperienceBuilder so any external callers of these deprecated macros
won’t error if they don’t pass xbProps.
| const deprecatedIceGroupCount = document.querySelectorAll('[data-craftercms-deprecated-ice-group]').length; | ||
| if (deprecatedIceGroupCount > 0) { | ||
| console.warn('\'iceGroup\' parameter is not supported anymore. Please refer to the documentation to migrate to the new macros: https://craftercms.com/docs/4.2/by-role/developer/upgrade/upgrading-in-context-editing.html#upgrading-in-context-editing'); | ||
| } |
There was a problem hiding this comment.
Only mark deprecated iceGroup when it is actually provided.
Line 77 emits data-craftercms-deprecated-ice-group even for the default empty iceGroup, so Lines 5-8 warn that iceGroup is unsupported for every iceAttr usage.
Proposed fix
- data-craftercms-deprecated-ice-group="${iceGroup}"
+ <#if iceGroup?has_content>
+ data-craftercms-deprecated-ice-group="${iceGroup}"
+ </#if>Also applies to: 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/webapp/WEB-INF/templates/system/common/cstudio-support.ftl` around
lines 5 - 8, The current warning is shown for every iceAttr because the template
always emits data-craftercms-deprecated-ice-group even when iceGroup is empty;
change the template so the data-craftercms-deprecated-ice-group attribute is
only output when iceGroup is actually provided/non-empty, and update the runtime
check (deprecatedIceGroupCount / selector) to rely on that conditional attribute
(or check the attribute's value) so console.warn only runs for elements where
iceGroup was explicitly set.
| <#local $model = component!contentModel> | ||
| <#if component?has_content> | ||
| <#-- Use the component object --> | ||
| data-studio-components-objectId="${component.objectId}" | ||
| data-studio-zone-content-type="${component['content-type']}" | ||
| <#local $collection = component[target]!{}> | ||
| <#else> | ||
| <#-- Use objectId for backwards compatibility --> | ||
| data-studio-components-objectId="${objectId}" | ||
| data-studio-zone-content-type="${contentModel['content-type']}" | ||
| <#local $collection = contentModel[target]!{}> | ||
| </#if> | ||
|
|
||
| data-craftercms-type="collection" | ||
| data-craftercms-model-path="${$model.storeUrl!"__PATH_NOT_FOUND__"}" | ||
| data-craftercms-model-id="${$model.objectId!"__ID_NOT_FOUND__"}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find componentContainerAttr usages that omit component=.
# Expected: Any matches are impacted by the empty-hash fallback issue.
rg -n -C2 --pcre2 '<@componentContainerAttr\b(?![^>]*\bcomponent\s*=)'Repository: craftercms/engine
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Find the macro definition for componentContainerAttr
rg -n "macro componentContainerAttr" -A 10Repository: craftercms/engine
Length of output: 1131
🏁 Script executed:
#!/bin/bash
# Find all usages of componentContainerAttr with flexible pattern
rg -n "componentContainerAttr" -B 2 -A 2Repository: craftercms/engine
Length of output: 582
🏁 Script executed:
#!/bin/bash
# Search for the file itself to verify it exists and check its content
find . -name "cstudio-support.ftl" -type fRepository: craftercms/engine
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Search for all `@componentContainerAttr` calls to see argument patterns
rg -n "@componentContainerAttr" -B 1 -A 5Repository: craftercms/engine
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for componentContainerAttr in any context
rg -i "componentcontainerattr" -nRepository: craftercms/engine
Length of output: 195
🏁 Script executed:
#!/bin/bash
# Find files that include or import cstudio-support.ftl
rg -n "cstudio-support" --type ftlRepository: craftercms/engine
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Find files that include/import cstudio-support.ftl
rg -n "cstudio-support" --glob="*.ftl"Repository: craftercms/engine
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for any componentContainerAttr macro calls with flexible pattern
rg -n "componentContainerAttr\s*\("Repository: craftercms/engine
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search all file types for any reference to cstudio-support
rg -n "cstudio-support"Repository: craftercms/engine
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look at other macros in the same file to understand invocation patterns
cat src/main/webapp/WEB-INF/templates/system/common/cstudio-support.ftlRepository: craftercms/engine
Length of output: 4855
Fix empty-hash fallback bug in componentContainerAttr macro.
The elvis operator on line 44 returns the empty-hash default {} instead of contentModel when the component argument is omitted. Since empty hashes are defined (not undefined) in FreeMarker, component!contentModel evaluates to {}, causing lines 50-51 to always emit __PATH_NOT_FOUND__ and __ID_NOT_FOUND__ fallback strings. The conditional check on line 45 correctly identifies the empty component but assigns the wrong value to $model beforehand.
Move the $model assignment into the conditional branches to match the pattern used in componentAttr and iceAttr macros in the same file:
Proposed fix
- <#local $model = component!contentModel>
<#if component?has_content>
+ <#local $model = component>
<#local $collection = component[target]!{}>
<#else>
+ <#local $model = contentModel>
<#local $collection = contentModel[target]!{}>
</#if>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <#local $model = component!contentModel> | |
| <#if component?has_content> | |
| <#-- Use the component object --> | |
| data-studio-components-objectId="${component.objectId}" | |
| data-studio-zone-content-type="${component['content-type']}" | |
| <#local $collection = component[target]!{}> | |
| <#else> | |
| <#-- Use objectId for backwards compatibility --> | |
| data-studio-components-objectId="${objectId}" | |
| data-studio-zone-content-type="${contentModel['content-type']}" | |
| <#local $collection = contentModel[target]!{}> | |
| </#if> | |
| data-craftercms-type="collection" | |
| data-craftercms-model-path="${$model.storeUrl!"__PATH_NOT_FOUND__"}" | |
| data-craftercms-model-id="${$model.objectId!"__ID_NOT_FOUND__"}" | |
| <#if component?has_content> | |
| <#local $model = component> | |
| <#local $collection = component[target]!{}> | |
| <#else> | |
| <#local $model = contentModel> | |
| <#local $collection = contentModel[target]!{}> | |
| </#if> | |
| data-craftercms-type="collection" | |
| data-craftercms-model-path="${$model.storeUrl!"__PATH_NOT_FOUND__"}" | |
| data-craftercms-model-id="${$model.objectId!"__ID_NOT_FOUND__"}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/webapp/WEB-INF/templates/system/common/cstudio-support.ftl` around
lines 44 - 53, The $model is assigned too early using the elvis default
(component!contentModel) which yields an empty hash when component is omitted;
remove the top-level "<#local $model = component!contentModel>" and instead set
$model inside the existing conditional: in the branch where
"component?has_content" assign "$model = component.contentModel" and
"$collection = component[target]!{}"; in the else branch assign "$model =
contentModel" and "$collection = contentModel[target]!{}" so the macro
(componentContainerAttr) mirrors the pattern used by componentAttr/iceAttr and
avoids the empty-hash fallback causing __PATH_NOT_FOUND__/__ID_NOT_FOUND__ to be
emitted.
| <script> | ||
| console.warn('Non-working macro \'ice\'. Please refer to the documentation to migrate to the new macros: https://craftercms.com/docs/4.2/by-role/developer/upgrade/upgrading-in-context-editing.html#upgrading-in-context-editing'); | ||
| </script> |
There was a problem hiding this comment.
Keep deprecation warning scripts out of delivery mode.
These inline scripts are emitted before any modePreview guard, so live pages using legacy macros now get extra scripts and may violate stricter CSP policies. Scope the warnings to preview/authoring mode.
Proposed fix
<#macro ice id="" component="" componentPath="">
- <script>
- console.warn('Non-working macro \'ice\'. Please refer to the documentation to migrate to the new macros: https://craftercms.com/docs/4.2/by-role/developer/upgrade/upgrading-in-context-editing.html#upgrading-in-context-editing');
- </script>
<#if modePreview>
+ <script>
+ console.warn('Non-working macro \'ice\'. Please refer to the documentation to migrate to the new macros: https://craftercms.com/docs/4.2/by-role/developer/upgrade/upgrading-in-context-editing.html#upgrading-in-context-editing');
+ </script>
<div data-studio-ice="${id}"></div>
</#if>
</#macro>
<#macro draggableComponent id="" component="" componentPath="">
- <script>
- console.warn('Non-working macro \'draggableComponent\'. Please refer to the documentation to migrate to the new macros: https://craftercms.com/docs/4.2/by-role/developer/upgrade/upgrading-in-context-editing.html#upgrading-in-context-editing');
- </script>
<#if modePreview>
+ <script>
+ console.warn('Non-working macro \'draggableComponent\'. Please refer to the documentation to migrate to the new macros: https://craftercms.com/docs/4.2/by-role/developer/upgrade/upgrading-in-context-editing.html#upgrading-in-context-editing');
+ </script>
<#if id != "" && component == "" && componentPath == ""> <#macro componentZone id="">
- <script>
- console.warn('Non-working macro \'componentZone\'. Please refer to the documentation to migrate to the new macros: https://craftercms.com/docs/4.2/by-role/developer/upgrade/upgrading-in-context-editing.html#upgrading-in-context-editing');
- </script>
+ <#if modePreview>
+ <script>
+ console.warn('Non-working macro \'componentZone\'. Please refer to the documentation to migrate to the new macros: https://craftercms.com/docs/4.2/by-role/developer/upgrade/upgrading-in-context-editing.html#upgrading-in-context-editing');
+ </script>
+ </#if>
<div class="cstudio-component-zone" id="zone-${id}">Also applies to: 92-94, 117-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/webapp/WEB-INF/templates/system/common/cstudio-support.ftl` around
lines 83 - 85, Wrap the inline deprecation console.warn scripts so they only run
in preview/authoring mode by guarding them with the existing modePreview check
(e.g. only emit or execute the script when modePreview is truthy); locate the
offending script that logs "Non-working macro 'ice'" and change it to run inside
that modePreview conditional, and apply the same change to the other occurrences
(the similar inline scripts at the other two locations referenced).
1 participant
craftercms/craftercms#7629
Summary by CodeRabbit
Refactor
Documentation