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

Support fk column with code_ or _code #296

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support fk column with code_ or _code #296

wants to merge 2 commits into from

Conversation

adipriyantobpn
Copy link
Contributor

Q A
Is bugfix? yes
New feature? no
Breaks BC? no
Tests pass? yes
Fixed issues my issues

Add support for foreign key column like code_supplier or supplier_code

@yii-bot
Copy link

yii-bot commented Sep 2, 2017

Thanks for posting in our issue tracker.
In order to properly assist you, we need additional information:

  • When does the issue occur?
  • What do you see?
  • What was the expected result?
  • Can you supply us with a stacktrace? (optional)
  • Do you have exact code to reproduce it? Maybe a PHPUnit tests that fails? (optional)

Thanks!

This is an automated comment, triggered by adding the label status:need more info.

@adipriyantobpn
Copy link
Contributor Author

AFAIK, it is also common to use code as prefix or suffix in foreign key column.

In my case, actually we are need to build REST API for a legacy desktop application.
As that application is developed by vendor, we only have authorization to access the database, but not change the database structure (as if changing the foreign key column from supplier_code into supplier_id).

It will be nice, if string code also can be cleaned-up in relation name generation, in addition to string id.
As in my case, the database contains almost 300 tables, which always have code prefix or suffix in its foreign key columns.

When does the issue occur?

August 2017, since at that time we'd just start the REST API development.

What do you see? And what was the expected result?

All relation name which it's foreign key has code prefix or suffix, is generated with Code in it.
For example for foreign key supplier_code, instead of getting getSupplier() for its relation function name, gii generate getSupplierCode()

@samdark samdark self-assigned this Sep 3, 2017
@samdark
Copy link
Member

samdark commented Sep 3, 2017

OK. Got it. I can't say that using code for the purpose is common.

Instead of introducing hardcoded $string_replacements it worth making a list of replacements a property in model generator and leaving its default value to ['id']. This way there will be no false positives and you'll be able to set your own custom replacements via gii config.

@adipriyantobpn
Copy link
Contributor Author

@samdark
Thank you so much for your feedback.
Noted. I'll make public property for that.

@dmirogin
Copy link
Contributor

dmirogin commented Sep 9, 2017

Please, write unit tests for this case.

} elseif (substr_compare($key, 'id', 0, 2, true) === 0) {
$key = ltrim(substr($key, 2, strlen($key)), '_');
foreach ($this->fkColumnIdentifiers as $str) {
if (!empty($key) && strcasecmp($key, $str)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's meant by strcasecmp here? Not equal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i think so.
Actually i just copy that line from if (!empty($key) && strcasecmp($key, 'id')) { and change id string into $str variable accordingly.

Is there anything that i should revise?

@adipriyantobpn
Copy link
Contributor Author

@dmirogin, as a matter of fact, at this time, i don't have much experience for writing unit testing yet.
But i would learn it for sure.

@samdark samdark added this to the 2.0.6 milestone Sep 12, 2017
@samdark samdark added the pr:request for unit tests Unit tests are needed. label Dec 22, 2017
@yii-bot
Copy link

yii-bot commented Dec 22, 2017

Thank you for putting effort in the improvement of the Yii framework.
We have reviewed your pull request.

In order for the framework and your solution to remain stable in the future, we have a unit test requirement in place. Therefore we can only accept your pull request if it is covered by unit tests.

Could you add these please?

Thanks!

P.S. If you have any questions about the creation of unit tests? Don't hesitate to ask for support. More information about unit tests

This is an automated comment, triggered by adding the label pr:request for unit tests.

@samdark samdark modified the milestones: 2.0.6, 2.0.7 Dec 22, 2017
@samdark samdark removed their assignment Dec 22, 2017
@samdark samdark modified the milestones: 2.0.7, 2.0.8 Apr 30, 2018
@samdark samdark modified the milestones: 2.0.8, 2.1.1 Dec 8, 2018
@samdark samdark modified the milestones: 2.1.1, 2.1.2 Aug 13, 2019
@NickvdMeij
Copy link
Contributor

@samdark these changes might have impact on #417 . was looking through the milestone issues and noticed this. Haven't checked it myself to be sure

@samdark samdark removed this from the 2.1.2 milestone Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:request for unit tests Unit tests are needed. type:enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants