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

Feature Add Oracle driver #2487

Merged
merged 184 commits into from
Jan 25, 2022
Merged

Conversation

ytetsuro
Copy link
Contributor

@ytetsuro ytetsuro commented Jan 14, 2020

Description
I wanted to use CodeIgniter4 in an Oracle environment.
Also, it was determined that support would be provided because ORACLE was described in the document.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

The design of the program to get the insertId of this PR is not so good.

Please good solution.

@ytetsuro ytetsuro force-pushed the add-oracle-driver branch 4 times, most recently from ea45b93 to 0f3d3d8 Compare January 14, 2020 06:30
@lonnieezell
Copy link
Member

Thanks so much for submitting this! You rock for that. I won't be able to get to this until after the next RC release, honestly. Besides reviewing this, we need to get Travis setup to be able to test it if possible. I'm honestly not familiar enough with Oracle to know if there's a simple to setup community version. Looks like there's Express Edition but would have to look deeper to see if we could integrate.

@ytetsuro
Copy link
Contributor Author

@lonnieezell
Thanks feedback.

Looks like there's Express Edition but would have to look deeper to see if we could integrate.

Humm...
it may be difficult.

oracle/docker-images#1156

Is there anything I can do to help with the this review?

@yogahilmi
Copy link

I tried using this driver with Oracle 18C, it works fine.

Thank you @ytetsuro

@kenjis
Copy link
Member

kenjis commented Jan 23, 2021

@ytetsuro @lonnieezell
This repo has GitHub workflows with Oracle image.
https://github.com/yiisoft/db-oracle

@michalsn
Copy link
Member

I was working on tests for this at some point and the main problem I had, was how to set an empty database because by default oracle comes with a lot of additional tables that break our tests. Also, I think we need at least Oracle 12 to support "auto-increment" values.

Maybe this thing with a fresh database is trivial but my DevOps skills are low 😅

@jkashwin
Copy link

jkashwin commented Apr 9, 2021

Any update on the availability of OCI8 driver support for CI4 ?

@javawebmedia
Copy link

I am really waiting for this Codeigniter 4 Oracle driver. Hopefully, it can be released soon. Thank you

@ytetsuro
Copy link
Contributor Author

@kenjis

This repo has GitHub workflows with Oracle images.

Very thanks.

Using the same idea, I checked the containers used in version 12c and above and found that quillbuilduser/oracle-18-xe was used.

The following repositories use this container.

@lonnieezell
@kenjis

we need to get Travis setup to be able to test it if possible

Changed GitHub Actions so that the OCI8 tests work with the above container.
Please review.

@kenjis kenjis added the database Issues or pull requests that affect the database layer label Sep 25, 2021
@kenjis
Copy link
Member

kenjis commented Sep 25, 2021

@ytetsuro @lonnieezell @michalsn @jkashwin @javawebmedia
What version of Oracle Database should this driver support?
12.1+?
12.2+?
18c+?
other?

And we need to add about this driver in
https://codeigniter4.github.io/CodeIgniter4/intro/requirements.html

@ytetsuro
Copy link
Contributor Author

ytetsuro commented Sep 25, 2021

@kenjis

Thanks for the quick feedback.

What version of Oracle Database should this driver support?

I think 12.1+.
I used the following version of this driver when I created it.

  • 12.1
  • 18c

The driver I created this time doesn't seem to use any of the features that were deprecated in 18c, so I think 12.1+ is fine.

https://docs.oracle.com/en/database/oracle/oracle-database/18/upgrd/behavior-change-deprecate-desupport-oracle-12c-rel-2.html#GUID-3BAFD95E-4D00-4F0F-BC80-6064F497F878

And we need to add about this driver in

Thanks.
I added it for now.

@kenjis
Copy link
Member

kenjis commented Jan 23, 2022

Database Driver abstracts Database operations, and this is the same for CodeIgniter4.

I'm not sure CI4 aims to Database Driver abstraction.
If we aim to it, we cannot use any Database specific functionality,
and cannot add any Database Driver specific functionality to a specific driver.
We should remove callFunction(), because it does not work if a user changes the database.

Personally I think abstraction is Query Builder (and CodeIgniter\Model) and Forge.
And Stored Procedure is not so rare functionality, but not all databases have it.
It might be possible that BaseConnection has storedProcedure().

  1. stop using original prepared statements when using OCI, and use a class that uses oci_bind_by_name to create queries.

I don't get what you mean. What is original prepared statements ?

@kenjis kenjis removed the stale Pull requests with conflicts label Jan 23, 2022
@ytetsuro
Copy link
Contributor Author

@kenjis

Thanks for feedback.

I'm learning a lot. Thank you.

I'm not sure CI4 aims to Database Driver abstraction.
If we aim to it, we cannot use any Database specific functionality,
and cannot add any Database Driver specific functionality to a specific driver.

Indeed, I don't think we should add it!

I thought that inheriting from BaseConnection meant that I was abstracting the Database connection.
I don't think we should do that even as a Liskov substitution principle.
If it is to be a subclass of BaseConnection.

However, the idea that you should use PDO if you want abstraction may also exist...

We should remove callFunction(), because it does not work if a user changes the database.
Personally I think abstraction is Query Builder (and CodeIgniter\Model) and Forge.

I don't know why there is a callFunction that can only return boolean.
However, as you said, abstraction may not be the goal when there are callFunction and query methods.

Thank you. I learned a lot and understand.

And Stored Procedure is not so rare functionality, but not all databases have it.
It might be possible that BaseConnection has storedProcedure().

It's true that storedProcedure is not an ORACLE-specific feature.
MySQL can call storedProcedure in a CALL statement via the query method.
In OCI, I think we need to prepare a separate storedProcedure method because we need to allocate variables of reference and resource types.
I think because the binding process for prepared statements is a proprietary implementation.
This is because references and resource types can only be bound using functions such as oci_bind_by_name.

From the above, I feel that it would be better to implement it in 2.
What do you think?

I don't get what you mean. What are original prepared statements?

I'm sorry.
My explanation was not good.
I meant to stop the original binding implementation for prepared statement.
Specifically, we are considering whether it would be possible to design the binding process in the Query class so that the PreparedQuery class is used for binding.
By doing so, we believe that users will be able to execute storedProcedure in the query method, just like in MySQL.

@kenjis
Copy link
Member

kenjis commented Jan 23, 2022

$ vendor/bin/rector process file1 file2 ...
$ composer cs-fix

@kenjis
Copy link
Member

kenjis commented Jan 23, 2022

I would like to release Oracle driver with functionality that can upgrade CI 3.1 code to 4.x as soon as possible.
If we can execute stored procedures by the query() method, Does it provide value to the Oracle users?

@ytetsuro
Copy link
Contributor Author

ytetsuro commented Jan 24, 2022

@kenjis

Thanks for feedback!

If we can execute stored procedures by the query() method, Does it provide value to the Oracle users?

No.

I can't offer much value in being able to run storedProcedure in the query method.
However, I think I can provide value in stopping proprietary bindings.
For example, it will be able to store binary data.
Being able to execute storedProcedure from the query method is one such example.

https://www.php.net/manual/en/function.oci-bind-by-name.php#example-1789

I would like to release Oracle driver with functionality that can upgrade CI 3.1 code to 4.x as soon as possible.

It might be a good idea to try to release it at this point and make it possible to do it in query, and then gently remove unnecessary methods in the 4.3 release.

@kenjis
Copy link
Member

kenjis commented Jan 24, 2022

@ytetsuro I don't fully understand your 2. plan, but it seems a good way to go.

But this PR is too long living and I really want to finish it.
Refactoring or improvements can be added later.

Can you fix the coding style error?

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

LGTM

@ytetsuro
Copy link
Contributor Author

@kenjis

Thank you very much for supporting this PR for a very long time.

I don't fully understand your 2. plan, but it seems a good way to go.

I understood.
I would like to issue an issue with more details on this content when this PR is merged.

Thank you very much!!

@kenjis kenjis merged commit 8863241 into codeigniter4:develop Jan 25, 2022
@kenjis
Copy link
Member

kenjis commented Jan 25, 2022

The one of the oldest, and most commented PR has been merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Issues or pull requests that affect the database layer dev new feature PRs for new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants