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

oci8: unused $seqField property #933

Closed
dregad opened this issue Mar 16, 2023 · 4 comments
Closed

oci8: unused $seqField property #933

dregad opened this issue Mar 16, 2023 · 4 comments
Assignees
Labels
bug OCI8 (Oracle) Oracle DBMS (Tier 1) PHP8.2 PHP8.2 Compatibility Issues
Milestone

Comments

@dregad
Copy link
Member

dregad commented Mar 16, 2023

I noticed this while working through #926, which was proposing to define the missing property to fix the PHP 8.2 deprecation notice:

protected function _insertID($table = '', $column = '')
{
if (!$this->seqField)
return false;

The seqField property is used in the data dictionary, but not referenced anywhere else in the driver.

This was introduced by you in #565, so maybe you'll remember what you were trying to achieve... If not I'd suggest to remove the if statement, as it feels like a mistake.

@mnewnham mnewnham added bug OCI8 (Oracle) Oracle DBMS (Tier 1) labels Mar 17, 2023
@mnewnham mnewnham added this to the v5.22.5 milestone Mar 17, 2023
@mnewnham
Copy link
Contributor

So here is what I was trying to achieve, I think. With oracle, there wasn't really the option to create autoincrement columns the way most other databases do, so what we have instead is the create record trigger that uses a sequence to do an on-insert update of an integer column that puts an incrementing value into the column.

The change I made then was because Johns original code started failing when Oracle allowed column names longer than (I think) 16 columns. I also tried to make the code portable so that if _insertId() was called and there wasn't an auto-increment column in the table, the function just returned immediately instead of doing the whole code flow including the getOne() at the end.

My mistake here was to confuse the code being executed for the dictionary for the stuff in the main system. I would like to have something there but thats clearly not the right approach.

@dregad
Copy link
Member Author

dregad commented Mar 17, 2023

Thanks for your reply. This what I thought but it's good to have confirmation.

To get rid of the PHP 8.2 deprecated warning, I propose to just remove the if statement for now since it's not doing anything useful.

If you want I can open a separate issue to track the need for detection of the auto-numbering sequence, and we can work on finding a proper fix for that later.

@dregad dregad changed the title oci8: unused $seqName property oci8: unused $seqField property Mar 17, 2023
@dregad dregad added the PHP8.2 PHP8.2 Compatibility Issues label Mar 17, 2023
@dregad
Copy link
Member Author

dregad commented Mar 17, 2023

If you want I can open a separate issue to track the need for detection of the auto-numbering sequence

Done, see #936

@dregad dregad assigned dregad and unassigned mnewnham Mar 17, 2023
@dregad dregad closed this as completed in 7e51937 Mar 17, 2023
@mnewnham
Copy link
Contributor

Ok thanks for looking at that. I started it but found that I couldn't get an oracle connection up and running at home. I will try to do something as soon as I can but I recognize that it's not ultra-important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug OCI8 (Oracle) Oracle DBMS (Tier 1) PHP8.2 PHP8.2 Compatibility Issues
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

2 participants