Domain bootstrap patch

You're right, it wasn't too hard to implement... attached is a patch with pretty much all the things I suggested above; please review! :)

Patch details:

• domain.admin.inc: new domain_alias settings page, added links to domain record listing
• domain.install: new schema. (I haven't changed domain_install() or added an update function)
• domain.module: modified domain_init() function, new menu callback, 2 new domain_alias functions, small addition to domain_domainload()
• domain_conf/settings_domain_conf.inc: modified to use bootstrap functions
• domain_prefix/settings_domain_prefix.inc: modified to use bootstrap functions
• domain_bootstrap.inc: new file, 3 functions: domain_resolve_host, _domain_lookup_simple, domain_current_domainname
• I included the changes from #270983: Domain Access doesn't work with non-standard port.

I think the Alias part should be split off into a separate module.

I didn't divide them because the aliasing part is tied into the domain core fairly deep, but it's probably better to separate the code... I'll give it a try later.

domain_bootstrap.inc. Includes the new file and changes to existing settings includes. My preference would be to run this from hook_init(), if possible, to avoid having to put items into settings.php.

Yes, I actually thought about that as well... the earliest hook that gets called is hook_boot() during DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE, which is after the config/variables and database phases... is that too late for domain_conf and domain_prefix?

If that works we wouldn't need a bootstrap file anymore as domain.module needs to be loaded for domain_boot... (still good for more clarity though, I guess)

Okay, thanks... I hope I'll manage to rewrite the code and produce a new patch by the beginning of next week.

If we move the domain lookup function out of hook_init(), we break hook_domainload(), because it seems that Drupal does not know what modules are installed when hook_boot() runs.

Yes...

It might be a good idea to have different version of the domain_load function...

• for example a bootstrap version, which can be used in domain_boot() or just from an settings-included file. (So basically the only requirement should be the database is up and running)
• ..and a full version, which can be used in domain_init(). This one would load all the information, call all the hooks etc..

What do you think?

-------------------
for reference (taken from drupal_bootstrap documentation):

1. DRUPAL_BOOTSTRAP_CONFIGURATION: initialize configuration.
2. DRUPAL_BOOTSTRAP_EARLY_PAGE_CACHE: try to call a non-database cache fetch routine.
3. DRUPAL_BOOTSTRAP_DATABASE: initialize database layer.
4. DRUPAL_BOOTSTRAP_ACCESS: identify and reject banned hosts.
5. DRUPAL_BOOTSTRAP_SESSION: initialize session handling.
6. DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE: load bootstrap.inc and module.inc, start the variable system and try to serve a page from the cache.This is where hook_boot is invoked
7. DRUPAL_BOOTSTRAP_LANGUAGE: identify the language used on the page.
8. DRUPAL_BOOTSTRAP_PATH: set $_GET['q'] to Drupal path of request.
9. DRUPAL_BOOTSTRAP_FULL: Drupal is fully loaded, validate and fix input data. This is where all modules are loaded and hook_init is invoked.

Be sure to test / patch against HEAD. I have been committing code steadily all weekend.

I'm checking out HEAD now and working from there...

Attached is a new patch introducing only domain_bootstrap.inc (patch against HEAD).

• Domain name resolving and basic lookup is zentralised, removing previous duplicate code...
• Only one file (settings.inc) needs to be included in settings.php, domain_conf_settings.inc etc. are not needed anymore, so the installation should be easier...
• Introduced two hooks which are invoked during domain bootstrap:
  • hook_domain_bootstrap_lookup($domain): allows to modify the domain_id resolving the current hostname... This is THE hook for the domain alias module.
  • hook_domain_bootstrap_full($domain): allows to do to more things after the domain bootstrap phases; This is THE hook for domain_prefix and domain_conf (see changes in the patch as well)
• Because it all happens before drupal's module and hook system is loaded, the modules implementing any of these hooks need to be "registered" manually in settings.inc.
• The code of course adds a little bit more overhead, but I've done a bit performance testing with timer_start() and timer_stop() and it doesn't look too bad.
• I have tested on a clean drupal 6.3 with domain_prefix/domain_conf on and off, and everything seems to work
• Documentation isn't completed yet, and install and readme files of course would need to be changed as well...

Feedback please... :)

I have attached a slightly modified version, mainly adding better comments and improving code quality.
To use the patch, apply it and change your settings.php to only include domain/settings.inc instead of those two settings includes for domain_conf and domain_prefix.

It would be great to just receive at least a little feedback if this is going to work as is (especially with those two "hooks"), so I can start developing the domain_alias submodule. (and if not I'm willing to keep working on it however long it takes until some version of this can be applied to the module) :)

Thanks for your reply :)

if ($www_replaced !== FALSE)
Just use if TRUE in both statements?

From php.net/strpos: Returns the position as an integer. If needle is not found, strpos() will return boolean FALSE.
Because I'm using it to check whether the "www." substring exists I'm only interested in whether it's not FALSE; the integer number zero is usually interpreted as FALSE as well which is the reason for the strict comparison.

DOMAIN_BOOTSTRAP_DNRESOLVE

Yes, that's not very clear... the idea behind the naming was "domain name resolve". (DNS = "domain name server" doesn't seem fitting.)
I'm renaming it to DOMAIN_BOOTSTRAP_DOMAINNAME_RESOLVE.

Not sure about registering modules in domain_settings.inc -- seems like a burden on the user. We can derive this information from the {system} table.

The idea behind that "registering" was to only load modules that implement those two domain_bootstrap (that is domain_prefix and domain_conf at the moment) hooks; this information is not stored in the {system} table. Just loading all modules doesn't seem like a good idea because it's actually too early in drupal's bootstrap phase. And just loading all modules that are marked for "bootstrap" might not include the relevant modules either.

The list of registered modules wouldn't have to be changed by the user, if a module can't be loaded it's just ignored. So that's only something that module developers would have to change by adding their modules if they implement one of the bootstrap "hooks".

An option to this would be to scan all modules for implementations of those hooks once in a while and cache the list somewhere (new database table?), but that seems much effort for probably no more than 3 or 4 modules using them. (This solution would be similiar to what drupal does -- marking modules implementing hook_boot in the {system} table.)

I admit it can be a little confusing to call them "hooks" while they are actually just a cheap copy of drupal's hooks... unfortunately we can't use drupal's hook system without initiating the whole module system (which I think can't be done without initiating those other early drupal bootstrap phases which we were trying to avoid in the first place...).

You can force module authors to declare this using hook_enable() and hook_disable(), then store it in a small table or variable that you query directly. Or we could store an array somewhere in the main module?

hook_enable() and hook_disable() is a good idea, I'll try that. Storing in a variable wouldn't work I think because domain bootstrap happens before variable_init()... So it's probably going to be a new table: {domain_system}

  $schema['domain_system'] = array(
    'description' => t("A list of modules that need to be loaded during domain bootstrap."),
    'fields' => array(
      'name' => array(
        'description' => t('The name of the item; e.g. domain_conf.'),
        'type' => 'varchar',
        'length' => 255,
        'not null' => TRUE,
        'default' => ''),
      'domain_bootstrap' => array(
        'description' => t("Boolean indicating whether this module is loaded during domain bootstrap."),
        'type' => 'int',
        'not null' => TRUE,
        'default' => 0),
      'weight' => array(
        'description' => t("The order in which this module's hooks should be invoked relative to other modules. Equal-weighted modules are ordered by name."),
        'type' => 'int',
        'not null' => TRUE,
        'default' => 0),
      ),
    );

I thought about altering the {system} table instead (just adding a new column domain_bootstrap would be enough) but it's probably better not to change drupal core stuff as long as it's not necessary, right?

Right, I didn't think about that... that's better than creating yet another new table.
Thanks, I'll quickly merge that with my code, do some tests and then I'll upload the new version...

Okay, I've implemented a version as you suggested, using the variable table and manually querying it. Other modules have to call domain_bootstrap_register('modulename') and domain_bootstrap_unregister('modulename') in their hook_enable() and hook_disable() implementations. By specifying a second parameter on domain_bootstrap_register they can also specify a weight.

I liked the idea of cleaning up the variable on the config screen, so I added a function domain_bootstrap_modules_cleanup() and a respective call to the hook implementation domain_domaininstall().

I also cleaned up all the comments again and removed debugging code.

Splitting this issue and moving the subdomain alias part to #284422: Subdomain Aliases...

New patch against current HEAD...

From quickly looking through the i18n patch I spotted at least two things that need to be adjusted:

• domain_init(): setting the (translated) site name probably needs to be done differently to work with the domain_bootstrap patch...
• changes in settings_domain_conf.inc need to be ported to hook_domain_bootstrap_full().

This definately needs some thorough testing... I probably won't get a chance to work on it during the next 2-3 weeks.

Maybe it would be good though to test and integrate this patch first before adding another feature... or the other way round ;-)

Which one is more important to you?

Personally this one (domain_bootstrap) as the domain alias module depends on it...

Hm, I'm not sure I understand what you mean but here's a new patch against HEAD... hope this one works :)

Hm, and I thought that it would work anyway... thanks for the lessson. :)
Attached is the patch following your naming example..

New Version patched against current HEAD and fixed an issue that resulted in the www. redirection not working properly.

How's the review process going? ;-)

I got some spare time this weekend, tell me if you need any help... :)

Applys nicely and everything seems to work... on a clean drupal6.4 anyway :)

A few "finishing" thoughts:

1. I don't really like the current implementation of domain_settings_setup_ok()...

I think it might be cleaner to rename it to something like "domain_check_settings_include" and have it handle the error message or error logging as well... At the moment, the same error message is sent from within bootstrap_full hooks in domain_prefix and domain_conf.

2. I haven't checked recently but I also remember having issues with error handling in the bootstrap file, because drupal_set_message and watchdog aren't loaded yet. We could handle that by storing errors in a static variable somewhere and handing them over to drupal's error handling during domain_boot() or domain_init().

3. Readme file(s) haven't been updated yet.

4. Bootstrap-hook comments (implementations and API.php) still include the following line:
In order for this hook to work your module needs to be registered in domain/settings.inc.

This should now be something like: In order for this hook to work your module needs to be registered as a domain bootstrap module by calling domain_bootstrap_register("modulename") in your hook_install implementation.

Wonderful, thank you very much! :)