Skip to content

5.next: Add ExistsInNullable #19040

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: 5.next
Choose a base branch
from
Draft

5.next: Add ExistsInNullable #19040

wants to merge 1 commit into from
+455 −0

Conversation

@dereuromark
Copy link
Member

@dereuromark dereuromark commented Nov 5, 2025 •
edited
Loading

Alternative to #18964

 public function buildRules(RulesChecker $rules): RulesChecker
  {
      // Allows null values in nullable composite foreign keys
      $rules->add($rules->existsInNullable(['author_id', 'site_id'], 'SiteAuthors'));
      return $rules;
  }

This is OK if we want to keep the ExistsIn and ExistsInNullable around for easier reusability.
If we only want ExistsIn with the switched config value in 6.x, then I would recommend going with a config flag instead.

TODO:

  • bake update to switch default one in use
  • docs update

@dereuromark dereuromark added this to the 5.3.0 milestone Nov 5, 2025
@dereuromark dereuromark marked this pull request as draft November 5, 2025 06:10
@markstory
Copy link
Member

markstory commented Nov 6, 2025

This is OK if we want to keep the ExistsIn and ExistsInNullable around for easier reusability.

I think this is a great solution. We let people opt-into the behavior they want with minimal fuss.

@dereuromark
Copy link
Member Author

dereuromark commented Nov 6, 2025

My main concern with this one (and why I prefer the opt-in config value in the other PR):

We dont want the Nullable part around in the future I think, only the ExistsIn (default) with the correct default (true).
Then people need to migrate a 2nd time so to speak.

Are we sure we want to introduce a new class here instead of a simpler approach I wonder.

@markstory
Copy link
Member

markstory commented Nov 8, 2025

We dont want the Nullable part around in the future I think

Why not? That class emulates how SQL unique keys work (for better or worse).

@dereuromark
Copy link
Member Author

dereuromark commented Nov 10, 2025 •
edited
Loading

Is that true?
From what I understand the auto nullable one (forward shimming in the other PR and then 6.x default) always applies correctly anyway.
It does rely on not abusing it for validation of course. The concerns shouldnt be mixed in the first place. Thats why the other PR solves this more cleanly, by addressing the root cause, not hiding the symptoms.

  • If validation is bypassed (patch entities, direct DB queries), null gets saved anyway
  • The domain rule's intent is to check foreign key existence, not nullability
  • Proper code would have explicit validation: ->notBlank('my_model_id')

Note: It also reduces upgrade efforts moving forward, e.g. adjustments of bake code and alike.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

5.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@dereuromark @markstory