In order to support multiple mixed realms (see #1559970: Battle plan / Progress) we are improving the Variable Realm API, including:

- Some new API functions (variable_realm_list(), etc...)
- Variables are auto loaded for realms using RealmController classes.
- Reusable form to configure variables for realm.
- Some new properties for hook_variable_realm_info().

This is the preliminary patch.

CommentFileSizeAuthor
#15 i18n_variable_realm-02.patch10.93 KBJose Reyero
#9 variable_realm_controller_class.patch40.34 KBJose Reyero
variable_realm_autoload.patch13.5 KBJose Reyero
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented
Issue tags: +Needs change record

Some more improvements committed:
- Realms are now capable of auto-loading, modules just need to do the 'realm switching'
- New hook_variable_realm_switch() and variable_settings_form_alter()
- Added a shared edit form for select variables for realm. Used in variable_realm_admin
- Improved main variable module so it can handle realms too if passed as an option.

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented

Variables in settings forms are now handled by variable_realm module.
(Needs cleaning up related modules and some more testing yet).

Realm conflicts are identified though not handled yet.

bforchhammer’s picture

bforchhammer CreditAttribution: bforchhammer commented
  1. Pages admin/config/system/variable/realm/* currently fail with Fatal error: Call to undefined function variable_realm_admin_variables_list_name
  2. variable_realm_form_key_selector uses drupal_strtolower on the realm title; this doesn't work in other languages; e.g. in German the first letter of realm name should stay in uppercase.
  3. not sure, but I think variable_realm_hook_info is missing some of the newly added hooks...
bforchhammer’s picture

bforchhammer CreditAttribution: bforchhammer commented
Assigned: Jose Reyero » bforchhammer

I'm going to work on these three issues now...

bforchhammer’s picture

bforchhammer CreditAttribution: bforchhammer commented

Here's another issue:

  1. Realm switcher does not work on: admin/config/system/variable/realm/%/edit
bforchhammer’s picture

bforchhammer CreditAttribution: bforchhammer commented
Assigned: bforchhammer » Jose Reyero

I have committed fixes for issues 1.-4. listed above, and also cleaned up the realm admin module (e.g. removed variable_realm_admin.inc which only contained functions that have been ported to the realm module or are no longer needed).

I also found three more issues:

  1. After saving a form, information from the "realm switcher" is lost, i.e. the user is redirected to the editing form for the default realm key. The initial patch solved this via the following snipped:
    // Make sure the form shown after submission is still the same one
$query_params = array();
foreach ($variables as $name => $realm) {
  $query_params[VARIABLE_REALM_SELECTOR_QUERY_PARAM_PREFIX . $realm] = $realms_current[$realm];
}
$form_state['redirect'] = array(current_path(), array('query' => $query_params));
  2. (minor) It would be nice if menu items were sorted by realm weight, i.e. the same sort order as in the realm overview table.
  3. (minor) For the case of very long lists of variables, it would be nice if variables on the realm settings page were sorted alphabetically (or possibly user-define by either name or group?)
Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented

Some more improvements (already in):
- Realms are sorted by weight in variable_realm_list(), used this order in variable realm admin menu.
- Added 'list callback' and 'keys callback' in variable_realm_info() to build this data on the fly using combined realms.
- Resolving conflicts in settings form using realm weights (variables in two realms are set for the .one with higher weight)
- Allow overriding realm weights with hidden variables 'variable_realm_weight_[realm_name]'

Now we just need to use these in variable_realm_union, domain_i18n_variable to handle these realm unions.

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented

Added hook_variable_realm_info_alter(&$realms);

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented
FileSize
variable_realm_controller_class.patch40.34 KB

This is one more step. As we start having too many callbacks and hook_realm_info() is getting too complex, this patch replaces most of it with a new class VariableRealmController, which is independent now of VariableRealmStore.

I think the code is shorter and cleaner.

bforchhammer’s picture

bforchhammer CreditAttribution: bforchhammer commented

Makes sense in terms of architecture; what does this mean for developers who want to create a new realm?

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented

Basically it means you can override the RealmController class and build any behavior you want. Since there are only two modules using realms now (i18n, domain), and we are introducing major API changes in this release anyway, I think it's the moment to make some improvements.

Still polishing the path a little bit, I'd like to have it ready today.

Also thinking about getting rid of the 'variable_realm_union' module and merging it into variable_realm, as it would become mostly a class that will be loaded upon demand.

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented

Created a new 7.x-2.x branch to work on this.

bforchhammer’s picture

bforchhammer CreditAttribution: bforchhammer commented

Sorry, I'm having a bit of a hard time keeping up with all the changes... ;-)

Basically it means you can override the RealmController class and build any behavior you want.

You can or you have to? Does this mean we can drop hook_variable_realm_info()? I guess it would be nice if we only had the controller hook and a custom class for a new realm. If that's the case that would probably require some refactoring in i18n_variable as well?

Created a new 7.x-2.x branch to work on this.

Good idea; especially if the release needs to be coordinated with a respective version of i18n_variable. I get the feeling that unit tests would also not be the worst idea to make sure things keep working...

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented

Well, I have the whole patch set (domain, i18n, domain_i18n) that I've been doing to test this changes so I'll be committing it (after tagging the prev version of domain_variable just in case).

You don't need to extend the class to create simple realms though if you need any of the callbacks (keys, lists, etc) you need to.

About variable_realm_info() I guess we could get rid of it, though the problem with this hooks is that the basic realm data (hook_realm_controller) is needed really early in the bootstrap (sometimes before hook_boot, like for domains), when no localization is available... And on the other side, that information you only need it to administer the realms, for naming things on the UI, etc so maybe we could invoke that hook once again for admin pages.

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented
FileSize
i18n_variable_realm-02.patch10.93 KB

@bforchhammer,

Following your idea, simplified that realm hooks, now everything is on hook_variable_realm_info() and looks much nicer and cleaner. Updated domain_variable accordingly.

Note for the configuration to be properly localized for admin pages we rebuild the static cache for realm info in variable_realm_init().

Attached patch for i18n_variable too.

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented
Status: Active » Fixed

Committed the i18n patch. Updated the domain_module for this changes.

Note we need to use now the 2.x branch of variable.

bforchhammer’s picture

bforchhammer CreditAttribution: bforchhammer commented

Nice, well done :)

I've created a few follow-up issues:

Automatically closed -- issue fixed for 2 weeks with no activity.