Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for non RecordLocale Entites when checking Owner Links #880

Open
wants to merge 2 commits into
base: 7
Choose a base branch
from

Conversation

MichaelHoughton
Copy link

@MichaelHoughton MichaelHoughton commented Aug 20, 2024

Description

This PR fixes issue on some modules where there are missing methods. The code was creating a bug such as:

Call to undefined method Purl\Fragment::getSourceLocale()

The bug is likely related to legacy Silverstripe applications, which have since been upgraded to SS 5.

In our context, we use Silverstripe for the Irish Parliament website.

Manual testing steps

Issues

  • Not sure how to reproduce this, however it appears to be related to building the site subtree in Silverstripe. It appears that not all entities which pass through Fluent are RecordLocale, and thus a 500 error is thrown on SS. This was fine when using SS4, but appears to have come after we upgraded to SS5.

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@@ -1068,7 +1068,7 @@ public function getLocaleInstances(): array
{
$locales = [];
foreach ($this->owner->Locales() as $info) {
if ($info->IsDraft()) {
if (method_exists($info, 'IsDraft') && $info->IsDraft()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if method_exists returns false, what is $info? Is it not an RecordLocale instance ?

@tractorcow
Copy link
Collaborator

Can you give more context on the error cases and what is causing this? I want to make sure I understand the cause before addressing the side effects. This fix is nice but might not address the core issue. E.g. do we need to use something other than $this->owner to query locale information?

@GuySartorelli
Copy link
Contributor

@MichaelHoughton I've added back the pull request template that you must have removed when you created this pull request. Please fill it out in full.

Note that it has a space for an issue - please create an issue, with details about the bug you're trying to fix including steps to reproduce it.
It's hard to tell for sure because there's not a lot of information in your PR description, but this sounds like it's probably a project-specific bug rather than one that should be addressed with a path in fluent.

@MichaelHoughton MichaelHoughton changed the title Custom fixes for missing methods Fix for non RecordLocale Entites when checking Owner Links Aug 21, 2024
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