Closed misog closed 2 weeks ago
Thanks. Feel free to open a PR. Otherwise, I'll look into this in the coming weeks.
Another bug that seems to be related to this: Blueprint can't handle well when a model has more than one relationship to the same table.
My YAML file:
models:
Client:
name: string
birth_date: datetime nullable
email: string nullable
mobile_phone: string,45 index
other_phone: string,45 nullable
cpf: string,20 nullable index
rg: string,20 nullable
facebook: string nullable
instagram: string nullable
Contact:
name: string
email: string
mobile_phone: string,45
client_id: id nullable
Quote:
quoted_value: string
description: text
date: datetime
client_id: id nullable
user_id_professional: id foreign:users.id nullable
user_id_attendant: id foreign:users.id nullable
user_id_creator: id foreign:users.id
Service:
price: string
description: text
date: datetime
client_id: id
quote_id: id nullable
user_id_professional: id foreign:users.id
user_id_attendant: id foreign:users.id nullable
user_id_creator: id foreign:users.id
and it builds the model's methods with the same name, ignoring the Laravel naming conventions. Generated Model Quote.php:
public function client(): BelongsTo
{
return $this->belongsTo(Client::class);
}
public function user(): BelongsTo
{
return $this->belongsTo(User::class);
}
public function user(): BelongsTo
{
return $this->belongsTo(User::class);
}
public function user(): BelongsTo
{
return $this->belongsTo(User::class);
}
and the generated migration file for the model quote:
Schema::create('quotes', function (Blueprint $table) {
$table->id();
$table->string('quoted_value');
$table->text('description');
$table->dateTime('date');
$table->foreignId('client_id')->nullable();
$table->foreignId('user_id_professional')->nullable()->constrained('users');
$table->foreignId('user_id_attendant')->nullable()->constrained('users');
$table->foreignId('user_id_creator')->constrained('users');
$table->timestamps();
});
I have been able to fix the method names locally, but I believe that this issue needs to be fixed first; otherwise, my PR will possibly cause some strange behavior.
Sorry, I'm not following. You referenced a PR or something than needs to be fixed first? Can you clarify what that is, or, ideally, provide a link?
Sorry, I'm not following. Did you reference a PR or something that needs to be fixed first? Can you clarify what that is, or, ideally, provide a link?
I was willing to fix the problem I mentioned and send a PR. So I cloned the repository and made a small correction (I haven't sent any commits yet). However, I believe that my changes will cause more problems than solutions. Anyway, I can do it and you can evaluate whether it is a valid change or not, or maybe send you the modified piece of code right here. I'm happy to help, but I've just started using the tool, and I think I need to understand a little more about how it works before making any changes to its core.
No worries. This is not something I have encountered. That's not to say it's not an issue. More that it might not fall under common use and as such something I welcome a PR for.
I decided to dive into the code and address the issues mentioned in this thread. I've just submitted a pull request with the changes and fixes I made, and I hope they will be helpful for the project.
A Few Important Notes: I believe that when we specify something in the YAML file, the tool should follow exactly what we want to do. However, it’s important to note that Blueprint is a Laravel package and should adhere to the framework's development standards.
In one of the examples mentioned above, it was suggested that the tool should add the relationship column in the migration even when it's the default 'id'. I respectfully disagree with this statement. One thing every Laravel developer can agree on is that the framework is here to facilitate and speed up development. Therefore, in the changes I made, the tool will only add the foreign key column information in the migration if it’s specified in the YAML file with dot notation, different from 'id', or outside of Laravel's naming conventions.
I also made it so the Lexer file internally creates an array specifically for relationships and foreign keys with all the specifications we add in the YAML file. This will ensure the tool follows exactly what we want without trying to guess things.
I am available to clarify any doubts and would like to reiterate what I said in another message: I don’t know the development of the tool in depth, so bugs may still occur, and if they do, you can count on me to help with whatever is needed.
Greetings from Brazil, Aaron
In your case, for me:
$table->foreignId('user_id')->constrained(); // this is wrong, should be constrained('user', 'id')
It looks right for me. There is laravel magic behind.
https://laravel.com/docs/11.x/migrations#foreign-key-constraints
Look at the second example:
Schema::table('posts', function (Blueprint $table) {
$table->foreignId('user_id')->constrained();
});
"foreignId" is not the same as the old "foreign", is taking care of the id and the table name
What's the final result of the table?
models: Offer: uuid: uuid text: text user_id: id foreign:user.id // explicit table and column
It's not actually ok because the yaml file is specifying that the relationship table is 'user' and not 'users' (or persons, in the second example). By omitting the table in the migration file the Laravel standard comes into action and the relationship constraint will be made based on the column name (user_id = table users)
This issue has become a bit mixed. What are we trying to solve here? I think maybe @AaronLil needs to open a separate issue and @misog should restate theirs.
Please follow the template with a concise description of the issue and the expect YAML and expected output.
jasonmccreary I don't think it's necessary. @misog reported that when he tried to use explicit foreign relationships, the blueprint just ignored what he put in the yaml file. This is probably because the tool automatically converts the table name to singular or plural when necessary to generate models or migrations. And he was right in thinking that this is a bug, because if we want the blueprint to create the relationship using the laravel standard, we just need to add the id and foreign without others modifiers and let all the magic happen. But since we're talking about the case where a table and/or id is specified, the correct thing to do is to generate the files with what we put in, and not try to guess what we meant.
If, for example, we specify user_id: id foreign:user.primary_key
(not users), the correct thing to do is to generate migrations and models with a relationship to the user table, and if it's wrong, just correct the declaration in the file and generate it again.
The main point of the fix I sent with the PR was what @misog reported. The rest of the fixes were just other bugs that are somehow related.
After the merge, when declaring country_id: id foreign:country.primary
the blueprint will create the table relationship to the country.primary
table and not to countries.primary
. And if it is country_id: id foreign
it will be the countries.id
, as the laravel standard dictates.
Yes, I think it is correct that Blueprint takes the dot syntax and converts it to a model. Blueprint is all about conventions, so I would not consider this a "bug". However, I do think if the dot syntax is used, more information could be extracted and used.
For example:
If someone wants to ensure those are implemented in a PR (with tests and preserving current Model reference behavior) I am willing to merge it. Otherwise, I do not intend to work on this issue based on Blueprint's adherence to Laravel conventions.
@jasonmccreary could you write blueprint line which will create foreign key with table "user" and column "id"?
@jasonmccreary I think we're missing the point here. The proposed change is not changing the standard for Models! Blueprint will continue to follow Laravel's naming standards (singular for classes, method names and relationships, etc.). The main change is in migrations, and at the end of the day that's what matters: creating an intermediate layer between the database structure and the development framework.
I strongly believe that we still need this change because many times it's not possible to use the Laravel standard, especially when we're creating a new application on top of an existing structure/application (which is my case). In these cases, Blueprint should be able to configure models and migrations with any definition that was placed in the yaml file. Maybe a smart option would be to use the $table property in the models, I don't know: https://laravel.com/docs/11.x/eloquent#table-names - "If your model's corresponding database table does not fit this convention, you may manually specify the model's table name by defining a table property on the model".
For now, the issue still exists. The user is specifying a table and the blueprint is simply ignoring this and creating the files pointing to the table it thinks is correct. So let's go to another example of how the blueprint works today without any changes/fixes:
models:
Client:
name: string
birth_date: datetime nullable
email: string nullable
mobile_phone: string,45 index
other_phone: string,45 nullable
cpf: string,20 nullable index
rg: string,20 nullable
facebook: string nullable
instagram: string nullable
relationships:
hasMany: Proposal, Service, Payment
Contact:
name: string
email: string
mobile_phone: string,45
client_id: id foreign nullable
Proposal:
price: string
description: text
date: datetime
client_id: id foreign nullable
user_id_professional: id foreign:usuarios.primary nullable
user_id_attendant: id foreign:usuarios.id nullable
user_id_creator: id foreign:usuarios
Service:
price: string
description: text
date: datetime
client_id: id foreign
proposal_id: id foreign nullable
user_id_professional: id foreign:users
user_id_attendant: id foreign:users nullable
user_id_creator: id foreign:users
relationships:
hasMany: Payment
Payment:
price: decimal
date: dateTime
description: text
service_id: id foreign
client_id: id foreign
user_id: id foreign
In the proposal model we are specifying that the relationship will be made in the 'usuarios' table (users in Portuguese).
I'm omitting some files, but some generated files that clearly have issues:
<?php
namespace App\Models;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsTo;
class Proposal extends Model
{
use HasFactory;
/**
* The attributes that are mass assignable.
*
* @var array
*/
protected $fillable = [
'price',
'description',
'date',
'client_id',
'user_id_professional',
'user_id_attendant',
'user_id_creator',
];
/**
* The attributes that should be cast to native types.
*
* @var array
*/
protected $casts = [
'id' => 'integer',
'date' => 'datetime',
'client_id' => 'integer',
'user_id_professional' => 'integer',
'user_id_attendant' => 'integer',
'user_id_creator' => 'integer',
];
public function client(): BelongsTo
{
return $this->belongsTo(Client::class);
}
public function usuario(): BelongsTo
{
return $this->belongsTo(Usuario::class, 'user_id_professional', 'primary');
}
public function user(): BelongsTo
{
return $this->belongsTo(Usuario::class);
}
public function user(): BelongsTo
{
return $this->belongsTo(Usuario::class);
}
}
Errors: 2 methods with the same name (user); there is no table or column information that the method is related to; created the correct method only when specifying a non-standard laravel id;
<?php
use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
return new class extends Migration
{
/**
* Run the migrations.
*/
public function up(): void
{
Schema::disableForeignKeyConstraints();
Schema::create('proposals', function (Blueprint $table) {
$table->id();
$table->string('price');
$table->text('description');
$table->dateTime('date');
$table->foreignId('client_id')->nullable()->constrained();
$table->foreignId('user_id_professional')->nullable()->constrained('usuarios', 'primary');
$table->foreignId('user_id_attendant')->nullable()->constrained('usuarios');
$table->foreignId('user_id_creator')->constrained('usuarios', 'creator');
$table->timestamps();
});
Schema::enableForeignKeyConstraints();
}
/**
* Reverse the migrations.
*/
public function down(): void
{
Schema::dropIfExists('proposals');
}
};
Errors: Totally delusional about the relationship column for user_id_creator, and since it was not specified in the yaml file, the correct one should just be 'id'; Omitted the 'id' for user_id_attendant (which is okay in my opinion);
Anyway, you are the maintainer of the project and the final word is yours, but the fixes are already done and ready to merge. In fact, all that's left is to update the tests so that the PR can pass the build process, which I honestly don't know much about.
Issue:
Explicit foreign:
This generates this schema:
This will try to create constrained foreign key to non-existing table
users
. It should create constrained foreign key to tableuser
instead. Specifyingprotected $table = 'user';
in User.php has no effect on the behaviour.Another issue:
This generates this schema:
While it is true that constrained('persons'); has the same effect on database as constrained('persons', 'id'); today - Shift should not deal with effects on database at all. It should act as translation layer between .yaml file and Laravel migration specs. One would want to set explicit column in migration files for various reasons. For example, there are migration text analyzers which do not deal with Laravel magic conventions, they deal with explicit options.
So, it tries to be smart when it should not be smart. It should be just dumb when explicit parameters are specified.