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

Feature: Add Config Option to Enforce Nullable Relationships #1580

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jeramyhing
Copy link

@jeramyhing jeramyhing commented Aug 7, 2024

Summary

This PR introduces a new configuration option enforce_nullable_relationships to the Laravel IDE Helper package. The purpose of this option is to provide flexibility in how nullable Eloquent relationships are treated during static analysis, helping to avoid false positives when the application logic ensures the presence of related records.

Rationale

The current behavior of the Laravel IDE Helper package can give unwanted null warnings in applications that ensure data integrity of relationships on non-nullable Eloquent relationship columns. Specifically, the static analysis tool may flag Eloquent relationship objects as nullable even when application logic guarantees that these relationships and their objects are never null. This behavior results in unnecessary and misleading warnings that disrupt development workflows.

This change addresses scenarios where the application logic guarantees the presence of related records, thus avoiding false positives in static analysis. By default, the option is set to true, preserving existing behavior, while allowing developers to disable it when needed.

Issue at Hand

  • False Positives: The existing logic flags Eloquent relationships as nullable, which is not desirable for applications that ensure these relationships are never null.
  • Development Inconvenience: Developers are forced to write superfluous code to appease the static analysis tool, adding unnecessary complexity and reducing code readability.
  • Performance and Integrity Considerations: Some applications choose not to use foreign keys for performance reasons while maintaining data integrity through application logic. The current behavior of the static analysis tool does not accommodate this scenario, leading to further inconvenience.

Changes Made

  1. Configuration Option: Added enforce_nullable_relationships to config/ide-helper.php
  2. Logic Update: Updated the isRelationNullable method in ModelsCommand.php to consider the new configuration option
  3. Test Update: Added new test cases in Tests/Console/ModelsCommand/Relations/Test.php to verify the behavior of the enforce_nullable_relationships configuration option
  4. Snapshot Update: Created a new snapshot to reflect the correct behavior when enforce_nullable_relationships is set to false.

Impact

  • Backward Compatible: The default behavior remains unchanged.
  • Flexibility: Developers can configure how nullable relationships are treated, reducing false positives in static analysis.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

Introduce enforce_nullable_relationships configuration to control nullable Eloquent relationships.
Update isRelationNullable method to respect new config option.
- Verify behavior of enforce_nullable_relationships configuration option.
- Create snapshot to reflect enforce_nullable_relationships set to false.
Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

LGTM!

Can you please add a changelog entry?

@barryvdh
Copy link
Owner

barryvdh commented Aug 7, 2024

But isn't it only marked as nullable when the database column is nullable? So if it cannot be null, it will not add |null right?

@jeramyhing
Copy link
Author

jeramyhing commented Aug 7, 2024

But isn't it only marked as nullable when the database column is nullable? So if it cannot be null, it will not add |null right?

That is not true for the relationship object. This PR (#1231) introduced a situation where the column is marked required (not-nullable) and the package will identify it as null because it does not have a foreign key defined. This causes an inconvenience for any application that chooses not to have foreign keys and uses not-nullable columns.

For clarification, it is the relationship object that is marked with |null not the foreign id column. See the configuration docblock for examples. I think there is a good case to have that behavior, but I also think it should support an option to turn that behavior off as well.

@johnbacon
Copy link

johnbacon commented Aug 26, 2024

We could use this, or something like this, for our use case.

In our database, we can't comfortably use foreign key constraints. Unfortunately, they lead to significant complications with schema changes -- namely, we can no longer use gh-ost (GitHub does not use and therefore support foreign key constraints) and native MariaDB ALTER ONLINE TABLE migrations also have significant limitations with foreign key constraints.

Additionally, we have a legacy codebase to support as we transition to Laravel, so we've got a whole host of things foreign key constraints would unfortunately complicate.

I know less about the correctness/incorrectness of the mentioned PR, but at least the option to forego foreign key constraints and rely solely on the related column's nullability would be of significant value to us.

Thank you!!

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.

4 participants