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

Code tidying #1134

Merged
merged 1 commit into from
May 23, 2024
Merged

Code tidying #1134

merged 1 commit into from
May 23, 2024

Conversation

Quendi6
Copy link
Contributor

@Quendi6 Quendi6 commented May 22, 2024

…(#1132)

Package targeted

Winter CMS

Description

If the query fails not because of a base table or view not found, the getSettingsRecord() function tries to access a $record variable that is not defined.

Here is an excerpt commenting on the changes:

    public function getSettingsRecord()
    {
        $query = $this->model->where('item', $this->recordCode);

        if ($this->cacheTtl > 0) {
            $query = $query->remember($this->cacheTtl, $this->getCacheKey());
        }

        $record = null; // PR: "$record = null;" defined outside of Try/Catch block.

        try {
            $record = $query->first();
        } catch (QueryException $ex) {
            // SQLSTATE[42S02]: Base table or view not found - migrations haven't run yet
            if ($ex->getCode() === '42S02') {
                // PR: "$record = null;" removed from this "if" case ("$record" already set to "null" if exception caught)
                traceLog($ex);
            }
        }

        return $record ?: null; // PR: We still need a condition to return "null" in case of empty collection.
    }

// PR: comments are here to explain my modifications.

Maybe I'm wrong, but I think it will be better code writing

Will this change be backwards-compatible?

This seems fully backwards compatible since the return will have the same values, the only difference being that it will become impossible to encounter the "Undefined variable $record" error.

@LukeTowers
Copy link
Member

@Quendi6 what was the exception that you ran into that triggered this error? I specifically wrote it like that originally so that anyone getting a different exception would have to report it to me 😂

@Quendi6
Copy link
Contributor Author

Quendi6 commented May 23, 2024

@LukeTowers 😂In truth: none. But the way it was written triggered something in me. All the code is clean except for this, which drives me crazy.🤷‍♂️ May be some OCDs here...

@LukeTowers LukeTowers changed the title Fix undefined variable $record when exception is not SQLSTATE[42S02] … Code tidying May 23, 2024
@LukeTowers LukeTowers merged commit 3ee9470 into wintercms:develop May 23, 2024
@LukeTowers LukeTowers added this to the 1.2.7 milestone May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants