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

Bugfix/crash on morph relations #12

Merged
merged 3 commits into from
Jun 7, 2024

Conversation

gael-connan-cybex
Copy link
Contributor

@gael-connan-cybex gael-connan-cybex commented Jun 4, 2024

PR Type

Bug fix, Enhancement


Description

  • Added conditional checks to prevent crashes when dealing with polymorph relations in higher Laravel 9 versions.
  • Refactored the code to provide less information about polymorph relations, ensuring compatibility and stability.

Changes walkthrough 📝

Relevant files
Bug fix
ModelReflector.php
Fix crash and enhance handling of polymorph relations       

src/ModelReflector.php

  • Added conditional checks for polymorph relations.
  • Prevented crash by handling methods that may not exist.
  • Refactored code to provide less information for certain relations.
  • +16/-10 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are moderate in size and complexity. The refactoring and addition of conditional checks are straightforward and localized to a specific functionality.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Missing Data: The refactoring introduces conditional checks which might lead to missing data fields ('relatedClass', 'relatedModel', etc.) in some cases if the method 'getForeignKeyName' does not exist. This could affect downstream processes expecting these fields.

    🔒 Security concerns

    No

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a check to ensure the method returns a valid relation instance before proceeding

    To avoid potential issues with methods that do not return a valid relation, add a check to
    ensure $relation is an instance of the expected relation class before proceeding.

    src/ModelReflector.php [101]

     $relation = $modelInstance->{$methodName}();
    +if (!($relation instanceof ExpectedRelationClass)) {
    +    continue;
    +}
     
    Suggestion importance[1-10]: 8

    Why: Adding a type check for $relation is crucial to prevent runtime errors from method calls on non-relation types, which is a significant improvement in robustness.

    8
    Add a null check for the return type to prevent potential null pointer exceptions

    To prevent potential null pointer exceptions, add a null check for
    $reflectionMethod->getReturnType() before calling getName() on it.

    src/ModelReflector.php [105]

    -'returnType' => $reflectionMethod->getReturnType()->getName(),
    +'returnType' => $reflectionMethod->getReturnType() ? $reflectionMethod->getReturnType()->getName() : 'unknown',
     
    Suggestion importance[1-10]: 8

    Why: Adding a null check before accessing methods on potentially null objects is crucial to prevent runtime exceptions, which can significantly impact the application's stability.

    8
    Maintainability
    Extract the logic for adding relation details into a separate method for better readability and maintainability

    To improve readability and maintainability, consider extracting the logic inside the if
    (method_exists($relation, 'getForeignKeyName')) block into a separate method.

    src/ModelReflector.php [109-119]

     if (method_exists($relation, 'getForeignKeyName')) {
    -    $relations->put($methodName, [
    -        ...$relations->get($methodName),
    -        'relatedClass'            => get_class($relatedModel),
    -        'relatedModel'            => $relatedModel,
    -        'relatedTable'            => $relatedModel->getTable(),
    -        'foreignKeyName'          => $relation->getForeignKeyName(),
    -        'qualifiedForeignKeyName' => $relation->getQualifiedForeignKeyName(),
    -        'isRelationParent'        => sprintf('%s.%s', $modelInstance->getTable(), $relation->getForeignKeyName()) !== $relation->getQualifiedForeignKeyName(),
    -    ]);
    +    $this->addRelationDetails($relations, $methodName, $relation, $relatedModel, $modelInstance);
     }
     
    Suggestion importance[1-10]: 7

    Why: Extracting complex logic into a separate method enhances readability and maintainability, though it's not a critical issue but a good practice in coding.

    7
    Performance
    Store the result of $relations->get($methodName) in a variable to avoid multiple calls and enhance performance

    To enhance performance, avoid calling $relations->get($methodName) multiple times by
    storing its result in a variable.

    src/ModelReflector.php [111-118]

    +$existingRelation = $relations->get($methodName);
     $relations->put($methodName, [
    -    ...$relations->get($methodName),
    +    ...$existingRelation,
         'relatedClass'            => get_class($relatedModel),
         'relatedModel'            => $relatedModel,
         'relatedTable'            => $relatedModel->getTable(),
         'foreignKeyName'          => $relation->getForeignKeyName(),
         'qualifiedForeignKeyName' => $relation->getQualifiedForeignKeyName(),
         'isRelationParent'        => sprintf('%s.%s', $modelInstance->getTable(), $relation->getForeignKeyName()) !== $relation->getQualifiedForeignKeyName(),
     ]);
     
    Suggestion importance[1-10]: 6

    Why: While this change improves performance by reducing redundant method calls, it's a minor optimization and not critical for functionality.

    6

    @gael-connan-cybex
    Copy link
    Contributor Author

    gael-connan-cybex commented Jun 4, 2024

    All (implemented) Retaylor buttons have been tested locally with this patch. The crashes around user management have demonstrably stopped occuring.

    also tested the api calls and ecom sync locally.

    src/ModelReflector.php Outdated Show resolved Hide resolved
    Copy link
    Contributor

    @mszulik mszulik left a comment

    Choose a reason for hiding this comment

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

    seems to work

    Copy link

    @jheusinger jheusinger left a comment

    Choose a reason for hiding this comment

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

    doesnt crash in the other app anymore

    @gael-connan-cybex gael-connan-cybex merged commit 46db78d into master Jun 7, 2024
    @gael-connan-cybex gael-connan-cybex deleted the bugfix/crash-on-morph-relations branch June 7, 2024 12:32
    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.

    Call to undefined method "getForeignKeyName" on Morph-Relations
    3 participants