Skip to content

Conversation

andrewnester
Copy link
Contributor

@andrewnester andrewnester commented Jul 28, 2017

Introduces new basic interfaces: PDOInterface and PDOStatementInterface.

Relates to https://bugs.php.net/bug.php?id=74957

@andrewnester
Copy link
Contributor Author

Link to internal discussion
http://news.php.net/php.internals/100067

@krakjoe krakjoe added the RFC label Jul 28, 2017
@andrewnester andrewnester force-pushed the 74957-pdo-interfaces branch from d69c698 to 2183684 Compare July 31, 2017 06:44
@nikic
Copy link
Member

nikic commented Aug 6, 2017

FYI most people probably don't receive your mails, due to the dmarc policy on bk.ru. It seems like this includes anyone using gmail and not specifically disabling spam checks on the internals list.

@andrewnester
Copy link
Contributor Author

@nikic wow, thanks a lot for information! need to do something about it

@HallofFamer
Copy link

It is an interesting idea, but I dont like the -Interface suffix. This is PHP Core, not PHP-FIG. I fail to see the point of following their convention in terms of interface naming, and even among PHP-FIG it was a controversial convention. There's gotta be a better way to name the interface.

@apapsch
Copy link

apapsch commented Aug 20, 2017

I used these names in the bug report because they were the most straightforward names, e.g. when you ask yourself "How can I implement custom PDO behaviour?" the natural OOP response is "implement the PDO interface"[0].

But I don't have too strong feelings for the names, because it will be easy to get from PDO class documentation to its interface docs. Other names I could think of:

  • DatabaseConnection / DatabaseStatement
  • DataObjectConnection / DataObjectStatement
  • DatabaseClient / DatabaseStatement
  • DataObjectClient / DataObjectStatement
  • DataData / DataDataData

Other ideas?

[0] That's also why any custom extension mechanism is bad in the OOP sense. Also, the custom mechanism will have to be learned and will likely differ from case to case.

@apapsch
Copy link

apapsch commented Jan 4, 2018

Seems @HallofFamer doesn't have strong feelings about the name either. It's been months now and no other comment. Looks like it could be merged?

@adambaratz
Copy link
Contributor

This PR was made in association with a potential RFC. Because of the changes involved, we'd want to vet it out more with the community before merging. Please see the thread linked at the top for more details. If you'd like to resume that conversation, the internals mail list is the right venue.

@nikic
Copy link
Member

nikic commented Feb 12, 2019

Full discussion: https://externals.io/message/100067

Having reread this just now, I think it can be summarized as:

  • We should allow ATTR_STATEMENT_CLASS to be used with persistent connections. This both appears to be not too hard technically, and is at least a large part of the motivation for this PR.
  • The overall reception for introducing interfaces does not seem to be very positive, partly because it seems unclear what the use-case is (beyond the previous point). Introducing the interfaces would need an RFC that convinces people to the contrary.

@apapsch
Copy link

apapsch commented Feb 12, 2019

The use case for an interface is the extensibility, being able to modify PDO behavior without going into cold C land (thanks to andrewnester for doing it). At the time of my bug report it seemed like a good idea. Since then, I created and used adapters of PDO, including Doctrine DBAL. The latter seems like a good place for a connection pool, which will solve my specific problem (losing connection to MySQL server). Continuing there.

Thus, I don't see the value of pursuing this PDO interface anymore.

@cmb69
Copy link
Member

cmb69 commented Mar 30, 2021

Thus, I don't see the value of pursuing this PDO interface anymore.

Well, than this PR can be closed, I think. Thanks for your work! :)

@cmb69 cmb69 closed this Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants