Add support to have lower case properites via a config variable. #1215

jwhulette · 2021-04-25T22:44:55Z

Summary

Allow column properties to be transformed to lower case via a configuration variable.

Type of change

New feature (non-breaking change which adds functionality)

Checklist

Existing tests have been adapted and/or new tests have been added
Add a CHANGELOG.md entry
Update the README.md
Code style has been fixed via composer fix-style

mfn · 2021-04-26T05:22:08Z

Please re-submit without any of the style changes
have you checked if it's possible using the new hooks? (Model hooks #945 and Allowing Methods to be set or unset in ModelHooks #1198 )

jwhulette · 2021-05-05T14:30:15Z

I have reverted the style changes.

I have looked into the hooks but I cannot see a way to change the case of the column name, as the hooks seem to be for adding additional information.

mfn · 2021-05-06T06:29:06Z

Allow column properties to be transformed to lower case via a configuration variable.

What I don't understand so far: is this a technical or cosmetic change?

ide helper just provides convenient IDE assistance, what is the different on $model->COLUMN was $model->column in terms of functionality with oracle? Are both cases working equally?

jwhulette · 2021-05-06T15:45:31Z

It's a bit of both.

When using the https://github.com/yajra/laravel-oci8 library, it returns the properties lower case by default.

Although it can be overridden, I prefer the lower case properties.

So, $model->COLUMN works if I manually set the properties to upper case, and $model->column works when using the default.

The issue is for methods. If using the default of uppercase, calling wherePAGENUMBER($value) results in the error: ORA-00904: "P_A_G_E_N_U_M_B_E_R": invalid identifier

Where as if the property is lower case, as wherePageNumber($value), it works properly.

Please, let me know if you have any further questions.

mfn · 2021-05-06T19:01:56Z

So this config change only makes sense together with https://github.com/yajra/laravel-oci8 ?

Just so I get this correct

Using regular oci8:

$model->COLUMN works
$model->column does not
$model->whereCOLUMN does not
$model->whereFoo ??

Using laravel-oci8:

$model->COLUMN does not work
$model->column works
$model->whereCOLUMN does not (basically: this can never work due to the transformation as you pointed out C_O_…
$model->whereFoo works

Unrelated to this, I don't think this should be enabled by default. And since you mentioned Oracle explicitly in the config documentation, I think it's fair to mention laravel-oci8 , as it seems to be required anyway for a sane integration.

Is this correct so far?

jwhulette · 2021-05-07T19:39:13Z

So this config change only makes sense together with https://github.com/yajra/laravel-oci8 ?
I believe it makes sense for any project that uses Oracle. As Oracle defaults to returning names as upper case.

Just so I get this correct

Using regular oci8:

Since laravel-oci8 uses the OCI8 under the hood I would assume the same results, but I have not tested regular OCI8 by itself.

* `$model->COLUMN` works

* `$model->column` does not

* `$model->whereCOLUMN` does not

* `$model->whereFoo` ??

Using laravel-oci8:

* `$model->COLUMN` does not work

This does work if I change the default case to upper, as the package defaults to converting them to lower

* `$model->column` works

Yes works

* `$model->whereCOLUMN` does not (basically: this can never work due to the transformation as you pointed out `C_O_…`

No, does not work

* `$model->whereFoo` works

Yes works

Unrelated to this, I don't think this should be enabled by default. And since you mentioned Oracle explicitly in the config documentation, I think it's fair to mention laravel-oci8 , as it seems to be required anyway for a sane integration.

Yes it should be defaulted to false, I have fixed the config file.

Is this correct so far?
Yes as noted.

Does this make sense?

mfn

I went once more through the PR and from my side, I'm ok with the general goal.

Looked now a bit closer and left some inline feedback, i.e. in the current version of the PR you accidentally removed hasLowerCaseProperties() so it doesn't really work.

Some other feedback: I appreciate you fixing the code style back, but it really doesn't make sense to keep your first commit like that. I ask you kindly if you can in the end just rebase and clean this up with a single proper commit; please also update against latest master in the meantime.

Lastly, please also add a changelog entry and see if you find a good spot in the README to mention the new config.

Thank you!

mfn · 2021-10-31T19:21:05Z

config/ide-helper.php

+     |--------------------------------------------------------------------------
+     | Some databases, like Oracle return the column names in upper case
+     |
+     | For example, normally you would see this:


Suggested change

| For example, normally you would see this:

| For example, normally with Oracle you would see this:

Updated doc block with your suggestion.

mfn · 2021-10-31T19:25:13Z

src/Console/ModelsCommand.php

@@ -442,6 +442,11 @@ public function getPropertiesFromTable($model)

        foreach ($columns as $column) {
            $name = $column->getName();
+
+            if ($this->hasLowerCaseProperties()) {


Something got b0rked with your last change, the method hasLowerCaseProperties isn't present anymore

Re-added the missing function

mfn · 2021-10-31T19:30:21Z

tests/Console/ModelsCommand/GenerateBasicPhpdocLower/Test.php

+        $app['config']->set('ide-helper.model_lower_case_properties', true);
+    }
+
+    public function test(): void


This test doesn't really perform the test you intended it to do, because none of the migrations provide upper case column definitions.

Added proper tests by creating column names that are uppercase

jwhulette · 2021-11-21T01:27:47Z

Rebased and updated Changelog. I didn't find a good spot in the readme.

mfn · 2021-11-21T18:40:08Z

src/Console/ModelsCommand.php

-use Doctrine\DBAL\Exception as DBALException;
-use Doctrine\DBAL\Types\Type;


These lines should not be removed 🤔

Sorry about that. I put the missing lines back.

…mn names are upper case

mfn

LGTM!

Ping @barryvdh , from my side good to merge

mfn · 2021-11-22T09:20:58Z

src/Console/ModelsCommand.php

+use Illuminate\Console\Command;
 use Doctrine\DBAL\Exception as DBALException;
 use Doctrine\DBAL\Types\Type;
-use Illuminate\Console\Command;


:sadpanda:

but fix-style will correct this, so nothing to do

mfn requested changes Oct 31, 2021

View reviewed changes

Rebase

f536df0

jwhulette force-pushed the Lower-case-column-properties branch 2 times, most recently from d095736 to 2fa2b3a Compare November 21, 2021 01:26

mfn requested changes Nov 21, 2021

View reviewed changes

Added option to set model properties to lower case when database colu…

7e9800b

…mn names are upper case

jwhulette force-pushed the Lower-case-column-properties branch from 2fa2b3a to 7e9800b Compare November 21, 2021 18:54

mfn approved these changes Nov 22, 2021

View reviewed changes

mdemori mentioned this pull request Oct 21, 2022

Problem with upper case DB Fields name on Oracle #1384

Open

jwhulette closed this Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to have lower case properites via a config variable. #1215

Add support to have lower case properites via a config variable. #1215

jwhulette commented Apr 25, 2021 •

edited by mfn

Loading

mfn commented Apr 26, 2021

jwhulette commented May 5, 2021

mfn commented May 6, 2021

jwhulette commented May 6, 2021

mfn commented May 6, 2021

jwhulette commented May 7, 2021 •

edited

Loading

mfn left a comment

mfn Oct 31, 2021

jwhulette Nov 21, 2021

mfn Oct 31, 2021

jwhulette Nov 21, 2021

mfn Oct 31, 2021

jwhulette Nov 21, 2021

jwhulette commented Nov 21, 2021

mfn Nov 21, 2021

jwhulette Nov 21, 2021

mfn left a comment

mfn Nov 22, 2021

	\| For example, normally you would see this:
	\| For example, normally with Oracle you would see this:

		use Doctrine\DBAL\Exception as DBALException;
		use Doctrine\DBAL\Types\Type;

Add support to have lower case properites via a config variable. #1215

Add support to have lower case properites via a config variable. #1215

Conversation

jwhulette commented Apr 25, 2021 • edited by mfn Loading

Summary

Type of change

Checklist

mfn commented Apr 26, 2021

jwhulette commented May 5, 2021

mfn commented May 6, 2021

jwhulette commented May 6, 2021

mfn commented May 6, 2021

jwhulette commented May 7, 2021 • edited Loading

mfn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhulette commented Nov 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhulette commented Apr 25, 2021 •

edited by mfn

Loading

jwhulette commented May 7, 2021 •

edited

Loading