barryvdh / laravel-ide-helper

IDE Helper for Laravel
MIT License
14.17k stars 1.16k forks source link

Feature: Add Config Option to Enforce Nullable Relationships #1580

Open jeramyhing opened 1 month ago

jeramyhing commented 1 month ago

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

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

Type of change

Checklist

barryvdh commented 1 month ago

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 commented 1 month ago

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 (https://github.com/barryvdh/laravel-ide-helper/pull/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 commented 1 month ago

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!!