Merge pull request #1179 from owen2345/fix/phase-4-session-shortcode-comment-helpers

Phase 4: Refactor session/shortcode/comment helpers to remove helper ivars

Author: texpert

.rubocop_todo.yml

@@ -151,8 +151,6 @@ Naming/VariableNumber:
 Rails/HelperInstanceVariable:
   Exclude:
     - 'app/helpers/camaleon_cms/frontend/nav_menu_helper.rb'
-    - 'app/helpers/camaleon_cms/session_helper.rb'
-    - 'app/helpers/camaleon_cms/short_code_helper.rb'
 
 # Offense count: 2
 Security/Eval:

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 ## Unreleased
 
+- **Refactor:** Replace Phase 4 session/shortcode/comment helper instance-variable state, [#1179](https://github.com/owen2345/camaleon-cms/pull/1179)
+  - Refactors `session_helper`, `short_code_helper`, and `comment_helper` to avoid helper ivar state via request-scoped `CurrentRequest` and explicit context passing
+  - Preserves controller/view compatibility points used by existing admin/session/shortcode flows
+  - Removes Phase 4 helper exclusions from `Rails/HelperInstanceVariable` in `.rubocop_todo.yml`
+  - Adds helper coverage for session and comment helpers and keeps shortcode helper coverage in place
+
 - **Refactor:** Replace Phase 3 admin/menu/taxonomy helper instance-variable state with CurrentRequest-backed state, [#1178](https://github.com/owen2345/camaleon-cms/pull/1178)
   - Refactors admin menus, post type, and custom fields helpers to use request-scoped CurrentRequest state
   - Eliminates traversal stack and registry instance variables from admin/menus, taxonomy hierarchy, and custom field helpers

app/helpers/camaleon_cms/comment_helper.rb

@@ -17,7 +17,10 @@ def cama_comments_get_common_data
 
     # render as html content all comments recursively
     # comments: collection of comments
-    def cama_comments_render_html(comments)
+    def cama_comments_render_html(comments, post_id = nil)
+      if post_id.nil? && respond_to?(:controller) && controller.respond_to?(:instance_variable_get)
+        post_id = controller.instance_variable_get(:@post)&.id
+      end
       comments.decorate.map do |comment|
         author = comment.the_author
         content_tag(:div, class: 'media') do
@@ -45,7 +48,7 @@ def cama_comments_render_html(comments)
                     content_tag(:div, class: 'pull-left') do
                       [
                         link_to(
-                          cama_admin_post_comment_answer_path(@post.id, comment.id),
+                          cama_admin_post_comment_answer_path(post_id, comment.id),
                           data: { comment_id: comment.id },
                           title: t('camaleon_cms.admin.comments.tooltip.reply_comment'),
                           class: 'btn btn-info reply btn-xs ajax_modal'
@@ -88,7 +91,7 @@ def cama_comments_render_html(comments)
                 end,
                 content_tag(:hr),
                 content_tag(:div, '', class: 'clearfix'),
-                cama_comments_render_html(comment.children)
+                cama_comments_render_html(comment.children, post_id)
               ].join.html_safe
             end
           ].join.html_safe

app/helpers/camaleon_cms/session_helper.rb

@@ -36,10 +36,11 @@ def login_user(user, remember_me = false, redirect_url = nil)
     # login a user using username and password
     # return boolean: true => authenticated, false => authentication failed
     def login_user_with_password(username, password)
-      @user = current_site.users.find_by(username: username)
-      r = { user: @user, params: params, password: password, captcha_validate: true }
+      user = current_site.users.find_by(username: username)
+      assign_controller_user(user)
+      r = { user: user, params: params, password: password, captcha_validate: true }
       hooks_run('user_before_login', r)
-      @user&.authenticate(password)
+      user&.authenticate(password)
     end
 
     # User registration.
@@ -51,20 +52,21 @@ def login_user_with_password(username, password)
     # - password
     # - password_confirmation
     def cama_register_user(user_data, meta)
-      @user = current_site.users.new(user_data)
-      r = { user: @user, params: params }
+      user = current_site.users.new(user_data)
+      assign_controller_user(user)
+      r = { user: user, params: params }
       hook_run('user_before_register', r)
 
       if current_site.security_user_register_captcha_enabled? && !cama_captcha_verified?
         { result: false, type: :captcha_error, message: t('camaleon_cms.admin.users.message.error_captcha') }
-      elsif @user.save
-        @user.set_metas(meta)
+      elsif user.save
+        user.set_metas(meta)
         message = if current_site.need_validate_email?
                     t('camaleon_cms.admin.users.message.created_pending_validate_email')
                   else
                     t('camaleon_cms.admin.users.message.created')
                   end
-        r = { user: @user, message: message, redirect_url: cama_admin_login_path }
+        r = { user: user, message: message, redirect_url: cama_admin_login_path }
         hooks_run('user_after_register', r)
         { result: true, message: r[:message], redirect_url: r[:redirect_url] }
       else
@@ -103,6 +105,7 @@ def cama_logout_user
       c_data = { value: nil, expires: 24.hours.ago }
       c_data[:domain] = :all if PluginRoutes.system_info['users_share_sites'].present? && CamaleonCms::Site.count > 1
       cookies[:auth_token] = c_data
+      CurrentRequest.user = nil
       redirect_to safe_redirect_url(params[:return_to]) || cama_admin_login_path,
                   notice: t('camaleon_cms.admin.logout.message.closed')
     end
@@ -122,16 +125,16 @@ def cama_current_role
 
     # return the current user logged in
     def cama_current_user
-      return @cama_current_user if defined?(@cama_current_user)
+      return CurrentRequest.user if CurrentRequest.user
 
       # api current user...
-      @cama_current_user = cama_calc_api_current_user
-      return @cama_current_user if @cama_current_user
+      current_user = cama_calc_api_current_user
+      return CurrentRequest.user = current_user if current_user
 
       return nil unless cookie_auth_token_complete?
 
-      @cama_current_user = current_site.users_include_admins
-                                       .find_by(auth_token: user_auth_token_from_cookie).try(:decorate)
+      users = current_site.users_include_admins
+      CurrentRequest.user = users.find_by(auth_token: user_auth_token_from_cookie).try(:decorate)
     end
 
     def cookie_auth_token_complete?
@@ -171,6 +174,13 @@ def cama_get_session_id
 
     private
 
+    def assign_controller_user(user)
+      target = respond_to?(:controller) ? controller : self
+      return unless target.respond_to?(:instance_variable_set)
+
+      target.instance_variable_set(:@user, user)
+    end
+
     # validate redirect url to prevent open redirect attacks
     def safe_redirect_url(url)
       return if url.blank?

app/helpers/camaleon_cms/short_code_helper.rb

@@ -2,9 +2,9 @@ module CamaleonCms
   module ShortCodeHelper
     # Internal method
     def shortcodes_init
-      @_shortcodes = []
-      @_shortcodes_template = {}
-      @_shortcodes_descr = {}
+      CurrentRequest.shortcodes = []
+      CurrentRequest.shortcodes_template = {}
+      CurrentRequest.shortcodes_descr = {}
 
       shortcode_add('widget', nil, "Renderize the widget content in this place.
                 Don't forget to create and copy the shortcode of your widgets in appearance -> widgets
@@ -80,22 +80,22 @@ def shortcodes_init
     # Also can be a function to execute that instead a render, sample: lambda{|attrs, args| return "my custom content" }
     # descr: description for shortcode
     def shortcode_add(key, template = nil, descr = '')
-      @_shortcodes << key
-      @_shortcodes_template = @_shortcodes_template.merge({ key.to_s => template }) if template.present?
-      @_shortcodes_descr[key] = descr if descr.present?
+      shortcode_keys << key
+      CurrentRequest.shortcodes_template = shortcode_templates.merge({ key.to_s => template }) if template.present?
+      shortcode_descriptions[key] = descr if descr.present?
     end
 
     # add or update shortcode template
     # key: chortcode key to add or update
     # template: template to render, if nil will render "shortcode_templates/<key>"
     def shortcode_change_template(key, template = nil)
-      @_shortcodes_template[key] = template
+      shortcode_templates[key] = template
     end
 
     # Delete the shortcode
     # key: chortcode key to delete
     def shortcode_delete(key)
-      @_shortcodes.delete(key)
+      shortcode_keys.delete(key)
     end
 
     # run all shortcodes in the content
@@ -138,6 +138,14 @@ def render_shortcode(text, key, template = nil)
       text
     end
 
+    def shortcodes_list
+      shortcode_keys
+    end
+
+    def shortcodes_descriptions
+      shortcode_descriptions
+    end
+
     private
 
     # helper to replace shortcodes adding support for closed shortcodes, sample: [title]my title[/title]
@@ -163,20 +171,33 @@ def _cama_replace_shortcode(content, item, args = {}, template = nil)
     def cama_reg_shortcode(codes = nil)
       # doesn't support for similar names, like: [media] and [media_gallery]
       # "(\\[(#{codes || (@_shortcodes || []).join("|")})(\s|\\]){0}(.*?)\\])"
-      "(\\[(#{codes || (@_shortcodes || []).join('|')})((\s)((?!\\]).)*|)\\])"
+      "(\\[(#{codes || shortcode_keys.join('|')})((\s)((?!\\]).)*|)\\])"
     end
 
     # determine the content to replace instead the shortcode
     # return string
     def _eval_shortcode(code, attrs, args = {}, template = nil)
-      template ||= @_shortcodes_template[code].presence || "camaleon_cms/shortcode_templates/#{code}"
-      if @_shortcodes_template[code].instance_of?(::Proc)
-        @_shortcodes_template[code].call(_shortcode_parse_attr(attrs), args)
+      templates = shortcode_templates
+      template ||= templates[code].presence || "camaleon_cms/shortcode_templates/#{code}"
+      if templates[code].instance_of?(::Proc)
+        templates[code].call(_shortcode_parse_attr(attrs), args)
       else
         render template: template, locals: { attributes: _shortcode_parse_attr(attrs), args: args }, formats: [:html]
       end
     end
 
+    def shortcode_keys
+      CurrentRequest.shortcodes ||= []
+    end
+
+    def shortcode_templates
+      CurrentRequest.shortcodes_template ||= {}
+    end
+
+    def shortcode_descriptions
+      CurrentRequest.shortcodes_descr ||= {}
+    end
+
     # parse the attributes of a shortcode
     def _shortcode_parse_attr(text)
       res = {}

app/models/current_request.rb

@@ -17,5 +17,6 @@ class CurrentRequest < ActiveSupport::CurrentAttributes
             :frontend_visited_home, :frontend_visited_post, :frontend_visited_ajax, :frontend_visited_search,
             :frontend_visited_post_type, :frontend_visited_tag, :frontend_visited_category,
             :frontend_visited_profile, :frontend_user,
-            :admin_menu_items, :custom_field_elements, :extra_models_for_fields
+            :admin_menu_items, :custom_field_elements, :extra_models_for_fields,
+            :shortcodes, :shortcodes_template, :shortcodes_descr
 end

app/views/camaleon_cms/admin/settings/shortcodes.html.erb

@@ -13,17 +13,17 @@
                     </tr>
                     </thead>
                     <tbody>
-                    <% @_shortcodes.each do |code| %>
+                    <% shortcodes_list.each do |code| %>
                         <tr>
                             <td>
                                 <code>[<%= code %>]</code>
                             </td>
-                            <td class="col_descr"><%= @_shortcodes_descr[code] %></td>
+                            <td class="col_descr"><%= shortcodes_descriptions[code] %></td>
                         </tr>
                     <% end %>
                     </tbody>
                 </table>
-                <%= content_tag("div", t('camaleon_cms.admin.message.data_found_list'), class: "alert alert-warning") if @_shortcodes.empty? %>
+                <%= content_tag("div", t('camaleon_cms.admin.message.data_found_list'), class: "alert alert-warning") if shortcodes_list.empty? %>
             </div>
         </div>
     </div>
@@ -34,4 +34,4 @@
             $(this).html($(this).html().replace(/\n/g, "<br>"));
         });
     })
-</script>
+</script>

docs/ai/plans/helper-ivar-refactor-master-plan.md

@@ -0,0 +1,52 @@
+# Refactor Plan: Replace Helper Instance Variables with Explicit State APIs
+
+Release context: `docs/ai/plans/releases/2.9.3.md`
+
+## Problem
+`Rails/HelperInstanceVariable` currently flags 160 offenses across 16 helper modules. Most of these helpers are stateful DSL/builders (menus, SEO, shortcode, content buffers, current object/site/session memoization), so removing ivars safely requires incremental API-preserving refactors with focused specs.
+
+## Proposed approach
+Refactor in small, behavior-safe phases (max 5 files/phase), introducing explicit state containers/memoized helper methods instead of ad-hoc instance variables. Keep public helper method signatures stable, add/expand helper specs per phase, and remove files from `Rails/HelperInstanceVariable` exclusions as each phase becomes clean.
+
+## Todos
+1. **prep-baseline-and-guardrails**
+   - Confirm current branch and helper offense scope.
+   - Document affected helper families and target phase order.
+   - Define the per-phase verification command set and rollback rule.
+
+2. **phase-1-content-and-asset-helpers**
+   - Refactor: `content_helper.rb`, `html_helper.rb`, `theme_helper.rb`, `hooks_helper.rb`.
+   - Replace ivar mutations/reads with explicit per-request state accessors.
+   - Add/update helper specs for content buffers and asset registries.
+   - Remove these files from `.rubocop_todo.yml` exclusion.
+
+3. **phase-2-frontend-context-helpers**
+   - Refactor: `frontend/content_select_helper.rb`, `frontend/seo_helper.rb`, `frontend/site_helper.rb`, `site_helper.rb`.
+   - Preserve nested block semantics (`process_in_block`) and current-site behavior.
+   - Add/update specs for object-context switching and SEO settings isolation.
+   - Remove these files from `.rubocop_todo.yml` exclusion.
+
+4. **phase-3-admin-menu-taxonomy-helpers**
+   - Refactor: `admin/menus_helper.rb`, `admin/post_type_helper.rb`, `admin/custom_fields_helper.rb`, `camaleon_helper.rb`.
+   - Replace temporary ivar traversal stacks with explicit local/context structures.
+   - Add/update specs for menu active-state resolution and taxonomy rendering.
+   - Remove these files from `.rubocop_todo.yml` exclusion.
+
+5. **phase-4-session-shortcode-and-remaining-helpers**
+   - Refactor: `session_helper.rb`, `short_code_helper.rb`, `comment_helper.rb`.
+   - Preserve login/session flows and shortcode registration/render behavior.
+   - Add/update specs for auth lookups and shortcode registry lifecycle.
+   - Remove these files from `.rubocop_todo.yml` exclusion.
+
+6. **phase-5-final-cleanup-and-ci-parity**
+   - Refactor remaining helper: `frontend/nav_menu_helper.rb`.
+   - Remove final `Rails/HelperInstanceVariable` helper exclusion(s) once compliant.
+   - Run full RuboCop and RSpec suite.
+   - Run `(cd spec/dummy && bin/rails zeitwerk:check)`.
+   - Delete `Rails/HelperInstanceVariable` exclusion block when empty.
+
+## Notes and considerations
+- Keep backwards compatibility for helper APIs used by themes/plugins.
+- Avoid large all-at-once rewrites; each phase should be independently mergeable.
+- Treat memoization separately from mutable builder state to avoid request leakage.
+- If a helper cannot be safely refactored without wider architectural change, stop and document a focused follow-up design decision.