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

Adopt PDO for database access #513

Open
jaragunde opened this issue Sep 16, 2021 · 11 comments
Open

Adopt PDO for database access #513

jaragunde opened this issue Sep 16, 2021 · 11 comments

Comments

@jaragunde
Copy link
Member

PHP Data Objects (PDO) define a consistent interface for accessing databases in PHP: https://www.php.net/manual/en/intro.pdo.php.

It can access different database providers with the same interface, saving ourselves the hassle to code DB access different implementations with pg_* or mysql_* methods if we need to support multiple backends.

More importantly, PDO provides a defense against SQL injection using parameter binding for DB queries. We currently have some protections using the DBAdapter code that we imported from the Propel project, but we must not forget to use them when coding queries and we probably failed to do so here and there.

Let's figure a way to adopt PDO incrementally in our code base!

@jaragunde
Copy link
Member Author

@jaragunde
Copy link
Member Author

jaragunde commented Nov 24, 2021

A lesson learned when coding the first patches: if we want to use PDO::FETCH_CLASS, we need to modify the VOs to have the internal properties match the column names.

I already attempted to match column names in the query with the internal properties in the VOs using AS like SELECT usrid AS userId, but it returns column names in lower case. The result is it creates a new userid property in TemplateVO, instead of assigning the correct value to the existing userId property.

jaragunde added a commit that referenced this issue Nov 25, 2021
Merge pull request #534 from Igalia/i513-pdo-for-template-table
@jaragunde
Copy link
Member Author

Reviewed in PR #534:

09a5efa6 [#513] Port Template DAO to use PDO. [Jacobo Aragunde Pérez]
9460159d Use PDO::FETCH_CLASS to retrieve TemplateVO objects. [Jacobo Aragunde Pérez]
f74cf305 Docs: Add PDO to system requirements. [Jacobo Aragunde Pérez]
01afc795 Port PostgreSQLTemplateDAO::delete() to PDO. [Jacobo Aragunde Pérez]
a4ea9bd5 Port PostgreSQLTemplateDAO::create() to PDO. [Jacobo Aragunde Pérez]

jaragunde added a commit that referenced this issue Dec 20, 2021
Merge pull request #539 from Igalia/i513-pdo-for-settings-table
@jaragunde
Copy link
Member Author

From #539:

f134ab10 [#513] Port Config DAO to use PDO. [Jacobo Aragunde Pérez]
81a521dd Port PostgreSQLConfigDAO to use PDO. [Jacobo Aragunde Pérez]
c868e06b Remove unused operation ConfigDAO::getVersionNumber(). [Jacobo Aragunde Pérez]
1b83e5c8 Move PDO object up in the DAO hierarchy to BaseDAO. [Jacobo Aragunde Pérez]
06f54e84 Extract PDO connection logic to DatabaseConnectionManager. [Jacobo Aragunde Pérez]

This was referenced Jan 12, 2022
jaragunde added a commit that referenced this issue Jan 25, 2022
Merge pull request #551 from Igalia/pdo-customer
jaragunde added a commit that referenced this issue Jan 25, 2022
Merge pull request #553 from Igalia/pdo-sector
@jaragunde
Copy link
Member Author

Reviewed in #551:

edd6c61b [#513] Port Customer DAO to use PDO. [Jacobo Aragunde Pérez]
4839173e Delete unused PostgreSQLCustomerDAO::setValues. [Jacobo Aragunde Pérez]
e4f3dbe9 Remove commented test lines in PostgreSQLCustomerDAO. [Jacobo Aragunde Pérez]
06a1e578 [#431] Fix PHP warnings in customer management page. [Jacobo Aragunde Pérez]
04bd1841 Migrate CustomerDAO create/update/delete operations to PDO. [Jacobo Aragunde Pérez]
0424e4b5 Remove unused CustomerDAO::getBySectorId and related code. [Jacobo Aragunde Pérez]
011987c6 Migrate CustomerDAO::getByProjectUserLogin to PDO. [Jacobo Aragunde Pérez]
6141862c Migrate CustomerDAO::getById and getAll to PDO. [Jacobo Aragunde Pérez]
5bd78abe Remove redundant CustomerDAO constructor declaration. [Jacobo Aragunde Pérez]
df4e6f2f Move runSelectQuery() up to BaseDAO. [Jacobo Aragunde Pérez]

Reviewed in #553:

edb994c7 [#513] Port Sector DAO to use PDO. [Jacobo Aragunde Pérez]
b8f9d25c Remove commented test lines in PostgreSQLSectorDAO. [Jacobo Aragunde Pérez]
d8b73be8 [#431] Fix 'undefined variable: string' in sector services. [Jacobo Aragunde Pérez]
06c20138 Migrate SectorDAO create/update/delete operations to PDO. [Jacobo Aragunde Pérez]
c2e78a38 Migrate SectorDAO::getById and getAll to PDO. [Jacobo Aragunde Pérez]
79b6d7d2 Remove redundant SectorDAO constructor. [Jacobo Aragunde Pérez]

jaragunde added a commit that referenced this issue Feb 9, 2022
Merge pull request #560 from Igalia/pdo-area
@jaragunde
Copy link
Member Author

Reviewed in PR #560:

ff8fc346 [#513] Port Area DAO to use PDO. [Jacobo Aragunde Pérez]
66486a9c Remove placeholder implementations of setValues in PDO DAOs. [Jacobo Aragunde Pérez]
3efef683 Remove commented test lines in PostgreSQLareaDAO. [Jacobo Aragunde Pérez]
bc8dbff6 [#431] Fix warnings in area-related services. [Jacobo Aragunde Pérez]
af485803 Migrate AreaDAO create/update/delete operations to PDO. [Jacobo Aragunde Pérez]
9ac65148 Remove unused AreaDAO::getByName(). [Jacobo Aragunde Pérez]
5dae0d54 Migrate AreaDAO getter operations to PDO. [Jacobo Aragunde Pérez]
f251a936 Remove redundant AreaDAO constructor. [Jacobo Aragunde Pérez]

jaragunde added a commit that referenced this issue Apr 19, 2022
Merge pull request #575 from Igalia/pdo-extra-hour
@jaragunde
Copy link
Member Author

Merged in #575:

0faa34dd [#513] Port Extra Hour DAO to use PDO. [Jacobo Aragunde Pérez]
1976c0e9 [#431] Fix PHP warnings in hour compensation management page. [Jacobo Aragunde Pérez]
1079ec7e Remove unused test lines. [Jacobo Aragunde Pérez]
e9a86663 Migrate ExtraHourDAO create/update/delete operations to PDO. [Jacobo Aragunde Pérez]
3ffd3638 Remove ExtraHourDAO::setValues(). [Jacobo Aragunde Pérez]
8ed8ba07 Migrate ExtraHourDAO::getLastByUserId and getAll to PDO. [Jacobo Aragunde Pérez]
575992f8 Remove unused ExtraHourDAO::getByUserId(). [Jacobo Aragunde Pérez]
77a6fa8e Migrate ExtraHourDAO::getById to PDO and adjust VO. [Jacobo Aragunde Pérez]
6b91b7c7 Remove unused constructor for ExtraHourDAO. [Jacobo Aragunde Pérez]

jaragunde added a commit that referenced this issue Jan 13, 2023
Merge pull request #606 from Igalia/pdo-city-dao
@jaragunde
Copy link
Member Author

jaragunde commented Feb 17, 2023

As part of PR #608, the create operation in ProjectDAO was migrated:

a968bd17 Merge pull request #608 from Igalia/bubble-up-sql-errors [Danielle M]
7ec2b2fe Fix string construction in service [Danielle Mayabb]
207ea879 (origin/bubble-up-sql-errors) Clean up formatting, use getters [Danielle Mayabb]
5bfa360e Clean up some xml so that messages are returned correctly [Danielle Mayabb]
a4175f5b Fix formatting/naming, return error code on error [Danielle Mayabb]
cfcdc447 Reload data store if creation fails [Danielle Mayabb]
a15d8cae Update save of dates, facade, and service [Danielle Mayabb]
d1940ea5 Add error handling for not null violation [Danielle Mayabb]
7762bffd Update object metadata and fix date conversion [Danielle Mayabb]
b9d4be4a Update copyright and remove trailing whitespace [Danielle Mayabb]
6c080269 Update var convention and delete whitespace [Danielle Mayabb]
90337f62 Expand exception handling in projectManagement.js [Danielle Mayabb]
6ad9871c Update createProjectsService to check new property isSuccessful [Danielle Mayabb]
074632f4 Update Project action, facade, and DAO [Danielle Mayabb]
d58e254f Update ProjectDAO create to use PDO and return OperationResult [Danielle Mayabb]
441c31a4 Add OperationResult class [Danielle Mayabb]

jaragunde added a commit that referenced this issue Feb 17, 2023
Since we had to rewrite the delete DAO operation, we did it in PDO
terms, addressing #513 too.
jaragunde added a commit that referenced this issue Feb 23, 2023
Since we had to rewrite the delete DAO operation, we did it in PDO
terms, addressing #513 too.
jaragunde added a commit that referenced this issue Feb 23, 2023
Since we had to rewrite the delete DAO operation, we did it in PDO
terms, addressing #513 too.
jaragunde added a commit that referenced this issue Feb 23, 2023
Also rewrite the operation in PDO terms, addressing #513.
@jaragunde
Copy link
Member Author

jaragunde commented Feb 23, 2023

In PR #614 I migrated the delete operation in ProjectDAO:

f0ae97d8 Delete unused test code. [Jacobo Aragunde Pérez]
3145b3b3 [#172] Delete user assignment when deleting a project. [Jacobo Aragunde Pérez]
55d87e67 [#496,#513] Return more specific errors on project deletion. [Jacobo Aragunde Pérez]
1b1aa84c Remove unused constructor for ProjectDAO. [Jacobo Aragunde Pérez]

jaragunde added a commit that referenced this issue Feb 24, 2023
Also rewrite the operation in PDO terms, addressing #513.
jaragunde added a commit that referenced this issue Feb 27, 2023
We had a specific operation in the backend to update only certain
fields of a project, but the UI was always sending the full project
data.

We think the ability of updating only some fields of the project is
not important, and we prefer to minimize the amount of code to
maintain. So we have:
* Switched the web service to the previously unused UpdateProjectAction
  and ProjectDAO::update.
* Modify ProjectDAO::update to detect specific errors and return an
  OperationResult, just like the partialUpdate operation did.
* Modify ProjectDAO::update to use PDO.
* Remove all code related to PartialUpdateProject.
jaragunde added a commit that referenced this issue Feb 28, 2023
Also rewrite the operation in PDO terms, addressing #513.
jaragunde added a commit that referenced this issue Feb 28, 2023
We had a specific operation in the backend to update only certain
fields of a project, but the UI was always sending the full project
data.

We think the ability of updating only some fields of the project is
not important, and we prefer to minimize the amount of code to
maintain. So we have:
* Switched the web service to the previously unused UpdateProjectAction
  and ProjectDAO::update.
* Modify ProjectDAO::update to detect specific errors and return an
  OperationResult, just like the partialUpdate operation did.
* Modify ProjectDAO::update to use PDO.
* Remove all code related to PartialUpdateProject.
@jaragunde
Copy link
Member Author

jaragunde commented Feb 28, 2023

In PR #615 I migrated update operations in ProjectDAO:

0aeaf6e6 [#496, #513] Replace project partial update operation. [Jacobo Aragunde Pérez]
3fe93733 [#496,#513] Return more specific errors on project update. [Jacobo Aragunde Pérez]
b7b339bd Simplify project create operation. [Jacobo Aragunde Pérez]

jaragunde added a commit that referenced this issue Feb 28, 2023
The DAO operation is also ported to PDO, addressing #513.
jaragunde added a commit that referenced this issue Mar 1, 2023
The DAO operation is also ported to PDO, addressing #513.
jaragunde added a commit that referenced this issue Mar 6, 2023
The DAO operation is also ported to PDO, addressing #513.
jaragunde added a commit that referenced this issue Mar 7, 2023
The DAO operation is also ported to PDO, addressing #513.
jaragunde added a commit that referenced this issue Mar 8, 2023
The DAO operation is also ported to PDO, addressing #513.
@jaragunde
Copy link
Member Author

Merged in #618:

82d4d972 [#496] Review error response codes in task operations. [Jacobo Aragunde Pérez]
ddbeb195 [#173] Make error messages more noticeable. [Jacobo Aragunde Pérez]
13e9c2b2 [#496,#513] Return more detailed errors on task delete. [Jacobo Aragunde Pérez]
e49f20e5 [#496] Return more detailed errors on task update. [Jacobo Aragunde Pérez]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant