Move the openid storage to the DIC so as it becomes injectable and allows us to use storage other than a sql based system.

Patch coming shortly.

CommentFileSizeAuthor
#1 openid-storage.patch17.84 KBmarcingy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Status: Active » Needs review
FileSize
17.84 KB
webchick’s picture

So in general, I know we're building storage abstraction layers around the big subsystems in core (e.g. entities, fields, files, etc.), and I understand that because it allows use of mongodb, etc. Don't really have a problem with that.

This, however, seems like it kinda crosses a line, because OpenID is just a stupid feature module, it's not a deep underlying subsystem. And it leaves me to wonder how far we really want to take this pluggable storage because the resulting code is *way* harder to understand, and if D8 becomes "you can never ever write a query directly without a layer of abstraction" that's not going to be remotely friendly to developers.

So this isn't a "no," but it's a "hmmm. is this really a good idea? / why is this needed?"

lotyrin’s picture

I disagree with #2.

The idea is that if we're going to allow for people to build Drupal sites on different types of storage, then "you can never ever write a query directly without a layer of abstraction" becomes true for Drupal (core), whether we like that or not.

Contrib and site custom code remain free to do as they will, and sites without SQL storage will simply not be able to use modules which rely upon it. The decision of whether to abstract or not is up to the maintainers of those modules and choice is good.

I agree that this kind of effort for openid may not belong in the core queue, but only equally as much as openid.module as a whole doesn't belong in Drupal core. As it is, core should aspire to storage abstraction, and openid.module is in core, so I'm +1 on this change.

jibran’s picture

Project: Drupal core » OpenID
Version: 8.x-dev » master
Component: openid.module » OpenID Client
Issue summary: View changes

Moving to OpenID queue.