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

Wrong capitals for class/files when table is uppercase #366

Closed
sdlins opened this issue May 24, 2018 · 23 comments
Closed

Wrong capitals for class/files when table is uppercase #366

sdlins opened this issue May 24, 2018 · 23 comments
Labels
important status:ready for adoption Feel free to implement this issue. type:enhancement Enhancement
Milestone

Comments

@sdlins
Copy link
Contributor

sdlins commented May 24, 2018

What steps will reproduce the problem?

yiisoft/yii2-gii uses BaseInflector::id2camel() to generate class names but, when getting upper case table names from db like MY_TABLE, id2camel() does not work properly.

If it is ok, I could do a PR removing id2camel() and doing an inline conversion inside Generator::generateClassNames().

What is the expected result?

'MY_TABLE' --> MyTable

What do you get instead?

'MY_TABLE' --> MYTABLE

Additional info

Q A
Yii version 2.0.15.1
PHP version 7.1.17
Operating system Win10
@sdlins
Copy link
Contributor Author

sdlins commented May 24, 2018

@samdark
Copy link
Member

samdark commented May 24, 2018

How would it work with MyTable? Names like that are common in PostgreSQL...

@sdlins
Copy link
Contributor Author

sdlins commented May 24, 2018

Not sure what you mean but I would just change the generated class and file names. The table set in getTableName would not change. I have this working for MSSQL and MySQL (I extended the Generator).

On the other hand, I must to say that I am not familiar with Postgre.

@samdark
Copy link
Member

samdark commented May 24, 2018

I mean expected result for MyTable is MyTable.

@sdlins
Copy link
Contributor Author

sdlins commented May 25, 2018

AFAICS, it would not change. The change would affect only MY_TABLE since we have all uppercase and underscores separating the words. MyTable would not be affected.

@samdark
Copy link
Member

samdark commented May 25, 2018

How would you implement that?

@sdlins
Copy link
Contributor Author

sdlins commented May 26, 2018

// I changed this in generateClassnames()
return $this->classNames[$fullTableName] = Inflector::id2camel($schemaName.$className, '_');

// For this (not beauty but works for my companie DBs [mysql, mssql])
return $this->classNames[$fullTableName] = strtr(ucwords(implode(' ', explode('_', strtolower($schemaName.$className)))), [' ' => '']);

// To implement it for MyTable I just would use Inflector::camel2words()
// (preferably in a bit more clean way than this)
return $this->classNames[$fullTableName] = strtr(ucwords(implode(' ', explode('_', strtolower(strtr(Inflector::camel2words($schemaName.$className), [' ' => '_']))))), [' ' => '']);

@samdark
Copy link
Member

samdark commented May 26, 2018

It won't work: https://3v4l.org/164si

@sdlins
Copy link
Contributor Author

sdlins commented May 26, 2018

May be your test function yiiCamelize() is wrong.
Correct should be like Inflector::camel2words():

public static function camel2words($name, $ucwords = true)
    {
        $label = strtolower(trim(str_replace([
            '-',
            '_',
            '.',
        ], ' ', preg_replace('/(?<![A-Z])[A-Z]/', ' \0', $name))));

        return $ucwords ? ucwords($label) : $label;
    }

@sdlins
Copy link
Contributor Author

sdlins commented May 26, 2018

Just tested here breaking it in parts:

$data = [
            'MY_TABLE' => 'MyTable',
            'MyTable' => 'MyTable',
            'my_table' => 'MyTable',
            'myTable' => 'MyTable',
        ];

        // strtr(ucwords(implode(' ', explode('_', strtolower(strtr(Inflector::camel2words($schemaName.$className), [' ' => '_']))))), [' ' => '']);
        function genNames($input) {
            $cc = Inflector::camel2words($input);
            $cca = strtr($cc, [' '=>'_']);
            $ccal = strtolower($cca);
            $ccala = explode('_', $ccal);
            $ccalai = implode(' ', $ccala);
            $ccalaiw = ucwords($ccalai);
            $final = strtr($ccalaiw, [' '=>'']);
            return $final;
        }

        $result = '';
        foreach ($data as $orig => $expected) {
            $finalName = genNames($orig);
            $result .= $orig . ' would be changed to ' . $finalName . '<hr>';
        }

        return $result;

Result:

MY_TABLE would be changed to MyTable
MyTable would be changed to MyTable
my_table would be changed to MyTable
myTable would be changed to MyTable

@samdark samdark added status:ready for adoption Feel free to implement this issue. type:enhancement Enhancement and removed status:under discussion labels May 26, 2018
@samdark samdark added this to the 2.0.8 milestone May 26, 2018
@samdark
Copy link
Member

samdark commented May 26, 2018

OK. That looks like it. Do you want to make a pull request?

@sdlins
Copy link
Contributor Author

sdlins commented May 29, 2018

Yeah, I am on it. I would already do but i had no time this weekend. I'll do this week. 👍

@sdlins
Copy link
Contributor Author

sdlins commented Jun 1, 2018

@samdark, should I create some tables with variated names (underlines, hyphens, uppers, etc) inside tests/data/sqlite.sql just to be able to create a test in tests/generators/ModelGeneratorTests.php or would you have a better approach?

@sdlins
Copy link
Contributor Author

sdlins commented Jun 1, 2018

I think I found a better approach: it would be to mock ModelGenerator overriding getTableNames() since I want to test only name conversions.

@samdark
Copy link
Member

samdark commented Jun 1, 2018

Yep, mock is better.

@sdlins
Copy link
Contributor Author

sdlins commented Jun 1, 2018

I did just basic tests for that specific method. So I just turned that method public to test against it. I know it is not the preffered way for tests but I hope it to be enough. If not, let me know.

@samdark samdark closed this as completed in ab8ba3b Jun 1, 2018
@jkovacsab
Copy link

This last update broke my application's ability to update the existing model files with old case, and instead it creates new models of a new filename and new classname. How do I configure Gii to revert to the previous functionality and preserve the DB table names as classnames and filenames?

My DB table name is ABBRMyTable on purpose. My expected Model class name and filename is ABBRMyTable, i.e. no change, but I get AbbrmyTable.

Another example is MyTableABBR, and I expected no change in the case, but I am getting MyTableAbbr.

Let me know how to resolve soon, so I can continue using Gii for model regeneration.

@sdlins
Copy link
Contributor Author

sdlins commented Jun 8, 2018

Models' names should be PSR compliant. That is why this change was done.

For your specific case (and may others), it could be implemented a new attribute to form like dont touch table name. But I am not sure that @samdark would agree with that. For now, you could just preview the generated model and copy/paste the lines in your current code.

BTW, keep in mind that Gii should be used primarily to "bootstrap" your models' code.

@samdark
Copy link
Member

samdark commented Jun 9, 2018

Yes, that could be a problem. Technically it's a breaking change so either next version should be major or we should make it configurable and not turned on by default.

@samdark samdark reopened this Jun 9, 2018
@samdark samdark added important and removed status:ready for adoption Feel free to implement this issue. labels Jun 9, 2018
@jkovacsab
Copy link

jkovacsab commented Jun 9, 2018 via email

@sdlins
Copy link
Contributor Author

sdlins commented Jun 9, 2018

@samdark, I can do it too, no problem. What about a bool form attribute like Use exact table name for class/files that would defaults to true? Do you have any other better idea?
Edit: sorry, I had not seen that it is not ready for adoption anymore. Not sure I could work on it. Let me know.

@samdark
Copy link
Member

samdark commented Jun 12, 2018

It should not be exact name, just the same way it was before and yes, it should be default. You could work on it.

@sdlins
Copy link
Contributor Author

sdlins commented Jun 13, 2018

PR done. Not sure if the name Force PSR Class Names is "cool" but seems good to me. However, suggestions for change it are welcome.

The feature is now optional and the checkbox is not even displayed in the form except when table name contains *. I also included a hint for the field.

lav45 added a commit to lav45/yii2-gii that referenced this issue Oct 24, 2018
* upstream/master:
  Remove useless import of `Yii` from CRUD generator search model template
  Fixes yiisoft#379: Fixed bug in view page where delete button not work well
  Fixes yiisoft#366: Option to allow standardized class names capitals in model generator
  Fixes yiisoft#327: Fixed bug in Model generator when $baseClass is an abstract class
  Fixes yiisoft#366: Better class and file names for uppercase tables
  Update composer.json (yiisoft#364)
  Fix codestyle (yiisoft#362)
  prepare for next release
  release version 2.0.7
  Register CSRF meta-tags dynamically
  Updated CHANGELOG [ci skip]
  Fixed rules generation for JSON columns
  Removed redundant line from license [skip ci]
  added database version to issue template (yiisoft#352) [skip ci]
  docs/guide-ja revised [ci skip] (yiisoft#349)
  Updated issue template [skip ci]
  Fixes yiisoft#185: Fix bug in Model generators when FKs pointing to non-existing tables
  Fixes yiisoft#340: Fix bug in CRUD SearchModel generator
  Require Yii 2.0.14 (yiisoft#339)

Conflicts:
	composer.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
important status:ready for adoption Feel free to implement this issue. type:enhancement Enhancement
Projects
None yet
Development

No branches or pull requests

3 participants