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

Add allow-alias-relations option to bake model command #913

Merged
merged 4 commits into from Mar 18, 2023

Conversation

passchn
Copy link
Contributor

@passchn passchn commented Mar 10, 2023

Since PR #842 which was merged into 2.7.1, the bake model command skips the generation of relation-code, if the table does not actually exist in the database.

This might be unwanted behaviour for some more edge-case scenarios, e.g. when one table is used for multiple alias relations.

E.g., if you have an addresses table, you might want to have a field delivery_address_id and invoice_address_id on your customers table, while using the same table and Address entity for both of them. You would just have to set the className option for the two relations, so that they both point to a field addresses.id.

Since 2.7.1, the relations are not baked anymore, because there is no actual delivery_addresses table in the database.

This PR introduces a new option --allow-alias-relations to skip the check for an existing table and to bake the relation code anyway.

Related issue:
#912

@dereuromark
Copy link
Member

dereuromark commented Mar 10, 2023

I wonder if --skip-relation-check might be clearer.
But lets wait on further feedback from others first.

In general I like having it, since it has less change of failing if there is a unresolvable kind of relation in there.
So a good fallback before having to do things manually in such edge cases.

@dereuromark dereuromark added this to the 2.x (CakePHP 4) milestone Mar 10, 2023
@passchn
Copy link
Contributor Author

passchn commented Mar 11, 2023

I agree! I also have a lot of tables where I just save an id to, say, a youtube video. I at some point started to name the fields youtubeid instead of youtube_id to prevent bake from generating wrong code. So I appreciate the new behaviour to check tables first.

What doesn't work with this PR though is e.g. if you have a users table with a delivery_address_id, invoice_address_id and, say, a twitter_id for whatever reason. Then you want to have two hasOne alibi-relations to addresses, but just a field for twitter_id.

@passchn
Copy link
Contributor Author

passchn commented Mar 11, 2023

Just checked the documentation. The term alias or table alias is already used for the name of each relation, also for "real" relations with existing tables: https://book.cakephp.org/4/en/orm/associations.html

So --allow-alias-relations is nonsensical.

@markstory
Copy link
Member

What about having the option name reflect that it will generate associations for tables that don't exist? Names like include-missing or include-all-associations might work better 🤷

@passchn
Copy link
Contributor Author

passchn commented Mar 13, 2023

Hmm.. if you save twitter user ids as twitter_id, that is not really a "missing" relation.

@passchn
Copy link
Contributor Author

passchn commented Mar 13, 2023

I personally would prefer having a word describing that kind of relation. "Alibi" is already taken. Maybe "Pseudo-Relation" but that sounds odd.

That name could also be used in the docs for the examples UnapprovedComments and SubCategories.

@dereuromark
Copy link
Member

I am still for --skip-relation-check to just reflect that the check will not be done, and any kind of such relation, existing or not is being added.

@passchn
Copy link
Contributor Author

passchn commented Mar 14, 2023

I renamed the option now.

@markstory markstory merged commit dc65c33 into cakephp:2.x Mar 18, 2023
13 checks passed
@markstory
Copy link
Member

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.

None yet

3 participants