From 5cbbd140ba6f94f40bb77e652449351f0814e09c Mon Sep 17 00:00:00 2001 From: Richard Kramer <34233066+richard-kramer@users.noreply.github.com> Date: Fri, 12 May 2023 03:40:04 +0200 Subject: [PATCH] Fix Zeitwerk conventions for `Refinery::Pages::Finder` (#3524) Resolves #3523 The issue stems from the definition of `Refinery::Pages::Finder` and its sibling classes (`Refinery::Pages::FinderByTitle` and so on). These are all defined beside each other in [`pages/lib/refinery/pages/finder.rb`](https://github.com/refinery/refinerycms/blob/aee49a603860bf7d5fdb1532b9add5f4e7f839f4/pages/lib/refinery/pages/finder.rb) which gets required in [`pages/app/models/refinery/page.rb`](https://github.com/refinery/refinerycms/blob/aee49a603860bf7d5fdb1532b9add5f4e7f839f4/pages/app/models/refinery/page.rb#L6). When reloading, Zeitwerk reloads the model, which in turn loads the finder again. But because the file is named `finder.rb`, only `Refinery::Pages::Finder` gets unloaded and reloaded again. All other classes stay loaded and would be patched by the reload. But because `Refinery::Pages::Finder` has been reloaded and initialized again, it got a new `object_id`, thus triggering the **`superclass mismatch`** error. The solution, that fixed the error, was to move all sibling-classes of `Refinery::Pages::Finder` into it, making them children of `Refinery::Pages::Finder`. Because they are only called inside `Refinery::Pages::Finder` itself, this *should* not make problems. --- pages/lib/refinery/pages/finder.rb | 175 ++++++++++++++--------------- 1 file changed, 87 insertions(+), 88 deletions(-) diff --git a/pages/lib/refinery/pages/finder.rb b/pages/lib/refinery/pages/finder.rb index fa030d7d96..f32af40a97 100644 --- a/pages/lib/refinery/pages/finder.rb +++ b/pages/lib/refinery/pages/finder.rb @@ -57,123 +57,122 @@ def translations_conditions(original_conditions) translations_conditions end - end + class FinderByTitle < Finder + def initialize(title) + @title = title + @conditions = default_conditions + end - class FinderByTitle < Finder - def initialize(title) - @title = title - @conditions = default_conditions - end + def default_conditions + { :title => title } + end - def default_conditions - { :title => title } + private + attr_accessor :title end - private - attr_accessor :title - end + class FinderBySlug < Finder + def initialize(slug, conditions) + @slug = slug + @conditions = default_conditions.merge(conditions) + end - class FinderBySlug < Finder - def initialize(slug, conditions) - @slug = slug - @conditions = default_conditions.merge(conditions) - end + def default_conditions + { + :locale => Refinery::I18n.frontend_locales.map(&:to_s), + :slug => slug + } + end - def default_conditions - { - :locale => Refinery::I18n.frontend_locales.map(&:to_s), - :slug => slug - } + private + attr_accessor :slug end - private - attr_accessor :slug - end - - class FinderByPath - def initialize(path) - @path = path - end + class FinderByPath + def initialize(path) + @path = path + end - def find - if slugs_scoped_by_parent? - FinderByScopedPath.new(path).find - else - FinderByUnscopedPath.new(path).find + def find + if slugs_scoped_by_parent? + FinderByScopedPath.new(path).find + else + FinderByUnscopedPath.new(path).find + end end - end - private - attr_accessor :path + private + attr_accessor :path - def slugs_scoped_by_parent? - ::Refinery::Pages.scope_slug_by_parent - end + def slugs_scoped_by_parent? + ::Refinery::Pages.scope_slug_by_parent + end - def by_slug(slug_path, conditions = {}) - Finder.by_slug(slug_path, conditions) + def by_slug(slug_path, conditions = {}) + Finder.by_slug(slug_path, conditions) + end end - end - class FinderByScopedPath < FinderByPath - def find - # With slugs scoped to the parent page we need to find a page by its full path. - # For example with about/example we would need to find 'about' and then its child - # called 'example' otherwise it may clash with another page called /example. - page = parent_page - while page && path_segments.any? do - page = next_page(page) + class FinderByScopedPath < FinderByPath + def find + # With slugs scoped to the parent page we need to find a page by its full path. + # For example with about/example we would need to find 'about' and then its child + # called 'example' otherwise it may clash with another page called /example. + page = parent_page + while page && path_segments.any? do + page = next_page(page) + end + page end - page - end - private + private - def path_segments - @path_segments ||= path.split('/').select(&:present?) - end + def path_segments + @path_segments ||= path.split('/').select(&:present?) + end - def parent_page - parent_page_segment = path_segments.shift - if parent_page_segment.friendly_id? - by_slug(parent_page_segment, :parent_id => nil).first - else - Page.find(parent_page_segment) + def parent_page + parent_page_segment = path_segments.shift + if parent_page_segment.friendly_id? + by_slug(parent_page_segment, :parent_id => nil).first + else + Page.find(parent_page_segment) + end end - end - def next_page(page) - slug_or_id = path_segments.shift - page.children.by_slug(slug_or_id).first || page.children.find(slug_or_id) + def next_page(page) + slug_or_id = path_segments.shift + page.children.by_slug(slug_or_id).first || page.children.find(slug_or_id) + end end - end - class FinderByUnscopedPath < FinderByPath - def find - by_slug(path).first + class FinderByUnscopedPath < FinderByPath + def find + by_slug(path).first + end end - end - class FinderByPathOrId - def initialize(path, id) - @path = path - @id = id - end + class FinderByPathOrId + def initialize(path, id) + @path = path + @id = id + end - def find - if path.present? - if path.friendly_id? - FinderByPath.new(path).find - else - Page.friendly.find(path) + def find + if path.present? + if path.friendly_id? + FinderByPath.new(path).find + else + Page.friendly.find(path) + end + elsif id.present? + Page.friendly.find(id) end - elsif id.present? - Page.friendly.find(id) end - end - private - attr_accessor :id, :path + private + attr_accessor :id, :path + end end end end