[11.x] Fix Rule::exists: Use correct table name on global models

[matiaslauriti]

I was working on a FormRequest's rule section at my company (using Laravel 9.x) when I tried using a normal Rule::exists(SomeModel::class, 'some_column')->withoutTrashed()->where('another_column', 1) but the rule was failing because it tried using database_name.SomeModel instead of the correct $table value for SomeModel.

I did check the source code and it does have ! str_contains($table, '\\') at the beginning of a check. I tried looking for any logic about why if it does not have \\ it should be used directly (when we do have a ! class_exists($table) after this check) but I did not find anything.

So, I followed these steps to accomplish the desired outcome:

1. Removed the previous logic mentioned from src/Illuminate/Validation/Rules/DatabaseRule.php
2. Run all the tests in tests/Validation/ValidationExistsRuleTest.php and all of them passed
3. Added that logic back
4. Added my new case inside the first test case
5. Ran all the tests again and my case failed (as expected)
6. Removed the first check again (main logic breaking the desired result)
7. Ran all the tests again and they all passed

I did have to do something super ugly I have never done, declare the whole test file inside a namespace (with { }), because I had to declare a test model on the global namespace and I didn't want to add it on the main structure of the repository, nor inside test folder and declare a new namespace on the composer file (psr-4 for autoload-dev).

Do let me know if there is a better way of declaring this global namespace (\) test model class for this specific test.

[taylorotwell]

I dunno, it's really hard for me to see what changed in this test given the diff. I'm also pretty hesitant of just removing code like this.

[matiaslauriti]

I dunno, it's really hard for me to see what changed in this test given the diff. I'm also pretty hesitant of just removing code like this.

@taylorotwell New lines are:

https://github.com/matiaslauriti/framework/blob/62712776e4d0809b276b3107db6ec394ff4fa68a/tests/Validation/ValidationExistsRuleTest.php#L73-L75

https://github.com/matiaslauriti/framework/blob/62712776e4d0809b276b3107db6ec394ff4fa68a/tests/Validation/ValidationExistsRuleTest.php#L348-L355

Is there a way you think I could achieve this without needing me to wrap the original file inside a namespace { }? I am also looking for a solution about it, but I was not able to find a better solution than that one.

I really want to commit this change, it would help a lot. And have in mind that if you remove the previously mentioned logic, no test fails, so that part is not tested.

Having \ or not having \ anywhere on the passed string would make no difference if ! class_exists($table) already returns false, because if it does not exist, that is the desired string to be used.

If the string has \ but the class does not exist, it will still return the same string exactly as it is, so that is redundant.

And if the string has \ and the class exists but it is not a model, it will still return the literal string as it is.

Following Blame, you can arrive at the original implementation: #30653 by @ahinkle, maybe he knows more and can help, because there are test cases missing too (I am doing my best trying to help with this 🙏).

[ahinkle]

Hi @matiaslauriti,

Interesting- I haven't run across someone using global models before. Could you share the rationale for adopting a global class modal? I'm not sure a PR would be accepted, implementing such a change might be challenging due to its potentially breaking nature, unless handled with some creativity.

[matiaslauriti]

Hi @matiaslauriti,

Interesting- I haven't run across someone using global models before. Could you share the rationale for adopting a global class modal? I'm not sure a PR would be accepted, implementing such a change might be challenging due to its potentially breaking nature, unless handled with some creativity.

Of course, thank you for your reply @ahinkle!

Currently, in the company I am working for, we do have some old code that was migrated to Laravel a long time ago, we do have "old" models that do not have a namespace, but we do have models that do follow the Laravel convention (\App\Models).

So, I was reviewing a PR and a coworker wrote the literal string rule for exists -> exists:table,column_to_relate_to,column_to_where_1,value_1,column_to_where_2,value_2. I could not remember by memory what was the column_to_where_1 and on. I checked the documentation and I found nothing about it so I went to the source code and checked it out.

I did remember and know that you can pass a Model::class and it will automatically infer the table name using $table->getTable(); so you do not need to care about the name changing in the future, it will always work as expected.

But when he tried the code I shared (Rule::exists(SomeModel::class, 'id')->withoutTrashed()->where('some_column', 1)), the call failed because it was trying to literally use the model's namespace. Because it is a global namespace, it literally used the class name (SomeModel) instead of the protected $table = 'another_table_name';.

My idea of this change is not only to have predictable code (it should work 100% with a Model, it does not matter where it is coming from, if it extends Model, it must use ->getTable()), but also let developers use that syntax (Rule::exists(Model::class)) on old code that may not exactly follow Laravel standards (but they are still Models).

The part that I am now more interested is not exactly "fixing" this unwanted behavior, but also getting to know why it makes use of ! str_contains($table, '\\') when it should simply be ! class_exists($table). I am very curious now, as the framework does not have tests (or I was not able to find any) related to the first statement (str_contains), so if I remove it, nothing fails.

I honestly would love to see this merged so it is predictable to anyone, but if it is not, I would also be happy if I or someone can add a test that finally identifies what str_contains is trying to catch or solve, adding more tests to that exact case, so the code base is more robust on this tiny detail ❤️

[matiaslauriti]

Hey @ahinkle! Sorry for the ping, just want to know if you were able to check what I wrote?

[matiaslauriti]

I see my code may not be relevant, but I am trying to contribute, fixing a small issue and adding tests, but it is hard to contribute on details like this when one no one can invest 5 minutes reading what I wrote on the description and the final code (super small)