propelorm / Propel2

Propel2 is an open-source high-performance Object-Relational Mapping (ORM) for modern PHP
http://propelorm.org/
MIT License
1.26k stars 399 forks source link

Fix issue with identifier quoting being ignored #1832

Closed oojacoboo closed 2 years ago

oojacoboo commented 2 years ago

This PR addresses a bug that prevents identifier quoting from being enabled when defined on the database node of the schema config. Some additional strict typing was added in the process.

codecov-commenter commented 2 years ago

Codecov Report

Merging #1832 (c7f605b) into master (d4c4392) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #1832   +/-   ##
=========================================
  Coverage     87.84%   87.84%           
+ Complexity     7766     7764    -2     
=========================================
  Files           226      226           
  Lines         21043    21043           
=========================================
  Hits          18486    18486           
  Misses         2557     2557           
Flag Coverage Δ
5-max 87.84% <100.00%> (ø)
7.4 87.84% <100.00%> (ø)
agnostic 67.12% <100.00%> (+<0.01%) :arrow_up:
mysql 69.11% <71.42%> (ø)
pgsql 69.12% <71.42%> (ø)
sqlite 66.94% <71.42%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nerator/Behavior/Archivable/ArchivableBehavior.php 91.46% <100.00%> (ø)
...rc/Propel/Generator/Behavior/I18n/I18nBehavior.php 95.45% <100.00%> (ø)
src/Propel/Generator/Model/Database.php 74.07% <100.00%> (+0.08%) :arrow_up:
src/Propel/Generator/Model/Table.php 92.30% <100.00%> (-0.02%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d4c4392...c7f605b. Read the comment docs.

oojacoboo commented 2 years ago

Okay, so, the main issue right now is that tests are written in such a way that the identifierQuoting defined on the table is the only support. However, identifierQuoting on the database node is documented and was not working. This PR has been written such that the table's identifierQuoting value is inherited from the database, if it's set. Otherwise, it uses the value of the table. So, if you defined identifierQuoting as true on the database, it'll apply to all tables. Otherwise, it'll take the value of the respective table.

Tests are failing because of this. Additionally, documentation on identifier quoting is very sparse and not very clear. As it presently stands, tests need to be modified for this change.

Is there any feedback on this?

dereuromark commented 2 years ago

It seems fair to adjust Tests in this case.

oojacoboo commented 2 years ago

Okay, so, I went with the existing test logic/intention - no need to modify tests.

Now, setting identifierQuoting at the database level applies to all tables of the database (defaults false). However, if identifierQuoting is set at the table level, it will override the database value, whether true or false. For example:

<database identifierQuoting="true">
    <table name="one" />
    <table name="two" identifierQuoting="false" />
    <table name="three" />
</database>

In this example tables, one and three would be quoted, table two would not.

<database>
    <table name="one" />
    <table name="two" identifierQuoting="true" />
    <table name="three" />
</database>

In this example, only the table, two, would be quoted.

The PR is now ready.

dereuromark commented 2 years ago

Are you able to provide an updated PR?

Also if possible

We can more easily verify each changeset and merge quicker.

oojacoboo commented 2 years ago

@dereuromark this should be ready now. Let me know if anything else is needed.