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

Move Drupal constants to Boot classes #1750

Closed
quicksketch opened this issue Nov 6, 2015 · 8 comments
Closed

Move Drupal constants to Boot classes #1750

quicksketch opened this issue Nov 6, 2015 · 8 comments

Comments

@quicksketch
Copy link
Contributor

We currently have this TODO: in bootstrap.inc:

// TODO: Move all of the constants below to a Drupal-specific file.
// We can't do this until commands are declaring which CMS they work
// with, because right now, commands that do not declare a 'bootstrap'
// level default to DRUSH_BOOTSTRAP_DRUPAL_LOGIN, so we need this constant,
// at least, available in non-Drupal contexts.

I couldn't find an issue for this, but I'm interested in helping test/implement this for the sake of the Backdrop community.

Related to removing deprecated constants: #1183

@quicksketch
Copy link
Contributor Author

One thing that I ran into quite quickly was whether these constants should remain in a global namespace like they are currently (DRUSH_BOOTSTRAP_DRUPAL_DATABASE) or if they should be switched to fully qualified names such as \Drush\Boot\DrupalBoot::BOOTSTRAP_DATABASE. Definitely more verbose to use the full names, but I suppose that's the "correct" approach?

@greg-1-anderson
Copy link
Member

We definitely want it to be easy for command authors to a) declare which framework they work with, and b) define their requirements, such as bootstrap levels, with a minimum of fuss.

One thing that is interesting is that some commands, e.g. the SQL commands, can be rather CMS-independent. All that the SQL commands require is a DB-URL, and they are good to go.

So, one proposal I have here is to define a new interface, \Drush\Boot\ProvidesSQLCredentials, that has a method that returns the db-url for the selected site. Each bootstrap class would implement this interface. The SQL commands command definitions could contain a new element that lists required interfaces, e.g. \Drush\Boot\ProvidesSQLCredentials, that must be implemented in the active Boot class in order for the command to be available.

This is, perhaps, a bad example, though, since the SQL commands can work if db-url is passed as a commandline option. There might be other useful examples, that use this pattern, though.

A different class of command are those that use engines to provide framework-specific functionality. The pm commands are one example of this. It would be nice if the command validation code could use the availability of an applicable engine to determine whether or not the given command is usable in the given context.

This is strongly related to #1749.

@quicksketch
Copy link
Contributor Author

One thing that is interesting is that some commands, e.g. the SQL commands, can be rather CMS-independent. All that the SQL commands require is a DB-URL, and they are good to go.

Yep, I've found that it's possible to make this work with the existing code if your Drush bootstrap phases inject the --databases option. I couldn't find a way to make Drush pick up a custom Sql class.

So, one proposal I have here is to define a new interface, \Drush\Boot\ProvidesSQLCredentials, that has a method that returns the db-url for the selected site. Each bootstrap class would implement this interface. The SQL commands command definitions could contain a new element that lists required interfaces, e.g. \Drush\Boot\ProvidesSQLCredentials, that must be implemented in the active Boot class in order for the command to be available.

This would be great. I think it's entirely sensible for a boot class to be able to identify the site's database connection(s). Then similar to #1724, we could make drush_sql_get_version() essentially a backwards-compatible wrapper around calling the boot class rather than manually creating a Sql class name based on the major Drupal version (Sql6, Sql7, etc).

Would this mean we'd nix the Sql6/7/8 classes entirely? It seems like it would be easy enough to replace their functionality with the newer boot classes. A little less abstraction might be good as well, merging together the controller-like classes for booting and SQL connections.

@greg-1-anderson
Copy link
Member

I don't remember where, but somewhere I recommended getting rid of the Sql6/7/8 classes, but on the other hand, we don't want the boot classes to become too bloated. It might also be possible to have the boot class just create an appropriate class similar to / same as the existing Sql classes. I would probably have an opinion on this if I tried to implement it. :)

@quicksketch
Copy link
Contributor Author

It might also be possible to have the boot class just create an appropriate class similar to / same as the existing Sql classes.

Yep, that's another easy option, just have the boot classes provide a method that returns an instance of the existing Sql classes instead. Rather than a interface for \Drush\Boot\ProvidesSQLCredentials, it could be \Drush\Boot\ProvidesSQLConnector (or something) that would point to a Sql class. However, with our current Sql classes having only 3 methods, it seems unnecessary. Either would be fine with me though, they'd both be better than magic instantiation based on drush_drupal_major_version().

@quicksketch
Copy link
Contributor Author

It looks like we actually have this magic version numbering going on for a number of classes:

  • Queue6/7/8
  • Role6/7/8
  • Sql6/7/8
  • StatusInfo6/7/8
  • User6/7/8
  • UserSingle6/7/8

Maybe we could make progress on all fronts if boot classes were responsible for loading all of these sub-classes that are version-specific? Then we could rework each of the various *_get_class() functions: drush_queue_get_class(), drush_sql_get_class(), drush_user_get_class(), etc. to pull from the boot class. drush_get_class() could also become deprecated if desired to end the magic class naming paradigm.

@greg-1-anderson
Copy link
Member

Yes, that's a good idea. Rather than making a special function for the SQL classes, the boot class should have a form of drush_get_class() that takes part of a class name, and knows how to find the specific implementation for its needs.

@quicksketch
Copy link
Contributor Author

We're getting a little off track here, so I made a new issue to discuss the instantiation of these classes from magic naming to the boot classes in #1766.

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

No branches or pull requests

2 participants