Skip to content

Conversation

@nicosp
Copy link
Contributor

@nicosp nicosp commented Jul 14, 2025

No description provided.

parent::initialize();
// Use our own table locator with fallback classes
$locator = new TableLocator();
$locator->allowFallbackClass(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, could we check if there is already one locator defined that has fallback activated?
Then we could skip adding another one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a way to check unless I am missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm. I figured it out. Not ideal but it works for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this looks like the safest BC way once we remove it from the app skeleton.

@nicosp nicosp requested a review from dereuromark July 14, 2025 13:33
@ADmad
Copy link
Member

ADmad commented Jul 14, 2025

Why the hacky try/catch and new instance creation instead of a single line $this->getTableLocator()->allowFallbackClass(true); ?

@dereuromark
Copy link
Member

If we remove the fallback in app skeleton, we might still have existing apps already declare it.
So declaring it twice seems overkill, no?

@ADmad
Copy link
Member

ADmad commented Jul 14, 2025

How is simply resetting a flag, even if already set, an overall, instead of recreating a locator instance?

@ADmad
Copy link
Member

ADmad commented Jul 14, 2025

Also replacing the existing locator instance with a new one would lose and customization/config. For e.g. one can set additional locations for the location using the $locations constructor argument or a custom QueryFactory.

@dereuromark
Copy link
Member

Yeah there should be no replacement. Only adding if not there yet

@dereuromark
Copy link
Member

@ADmad you might have misread. It does not replace.

@ADmad
Copy link
Member

ADmad commented Jul 15, 2025

you might have misread. It does not replace.

            $locator = new TableLocator();
            $locator->allowFallbackClass(true);
            $this->setTableLocator($locator);

These lines do replace the locator instance. All that needs to be done is toggle on the fallback behavior.

@ADmad
Copy link
Member

ADmad commented Jul 15, 2025

Closing in favor of #1047

@ADmad ADmad closed this Jul 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants