Suggested fixes

[herbdool]

Hi Vincent, good work! I took a look at the code but haven't tested. I've got a few small suggestions, mostly based on https://api.backdropcms.org/converting-modules-from-drupal.

In .install you'll need to remove any Drupal update hooks and add:

/**
 * Implements hook_update_last_removed().
 */
function modulename_update_last_removed() {
  return 7002;
}

to ensure it runs the right updates.

If you're using config_get() multiple times in a function consider using this to make it more efficient:

// Get the full config object
$config = config('modulename.settings');

// Get individual settings
$value_foo = $config->get('foo');
$value_next = $config->get('next');

You can remove test class from hook_autoload_info since tests are loaded only when running the testing framework and you don't want it to load all the time.

The module still includes code that relies on the PHP module which was removed in Backdrop: https://github.com/vstemen/css_injector/blob/2640f8fb3f277e03d2c7ecdc3a0ef9f7b067d47e/css_injector.admin.inc#L206 It's possible that someone could create a contrib module with the same name but I suppose right now that code isn't doing anything.

[vstemen]

Thanks for the suggestions.

About hook_update_last_removed().

I was still a bit in the fog about how the hook_update functions work as I did the port. I think I'm beginning to get a clearer picture.

I was originally unclear as to whether to leave in the css_injector_update_7002() function because the docs for hook_update_N() said

For the sake of compatibility, Backdrop will run any remaining 7xxx updates
before running 1xxx updates. 

I am still unclear on a couple of things. Assuming I remove that function, what exactly is the purpose of adding the hook_update_last_removed() call? Is it in case we bring any additional database changes over from Drupal 7 in the future, so that Backdrop will have recorded what the last update was?

If so, in the case of the Backdrop port, does it really matter? That update was never performed under Backdrop. I found this comment about it.

https://drupal.stackexchange.com/questions/141676/can-i-delete-old-hook-update-n-functions

Drupal doesn't care for the update numbers; it just runs the update
functions starting from the ones with the lower number.

If that's the case, then it seems to me that the hook_update_last_removed() call is unnecessary in this case, because we know that the database schema is already going to contain those fields from the initial port.

Or am I completely missing something?

If you're using config_get() multiple times in a function consider using this to make it more efficient:

Yes, I was aware of this. However, the places where I used config_get(), I was only getting a single variable.

[herbdool]

If you're calling one variable multiple times it's still more efficient to use the alternative approach, because it doesn't need to read the file multiple times.

From the documentation for hook_update_last_removed(): "If you remove some update functions from your mymodule.install file, you should notify Backdrop of those missing functions. This way, Backdrop can ensure that no update is accidentally skipped."

So I think it's a way of ensuring the module was at the latest Drupal 7 version before updating to Backdrop.

[vstemen]

This way, Backdrop can ensure that no update is accidentally skipped.

OK. Since the database is being updated with a schema that already has those updates on the initial port, it still looks to me to be technically unnecessary. However, I can see it possibly being useful for knowing which update_N() hooks to bring across if merging any new code from the Drupal branch in the future. So I will go ahead and add the update_last_removed() call.

If you're calling one variable multiple times it's still more efficient to use the alternative approach, because it doesn't need to read the file multiple times.

True, but the few places that the variables are being read are in different functions in different parts of the code. I would have to make the $config object global to make any difference wouldn't I? Because local variables and objects in each function are going to be deleted and the files closed when it returns from each function.

Anyway, with the small number of reads in this case, It might make the difference between the blink of an eye and 1.001 times the blink of an eye :-).

[herbdool]

In css_injector_admin() there's more than one call to config_get() to the same file, that's it I think. Yeah, I don't think making it a global makes sense, just of they're in the same function.

[herbdool]

@jenlampton perhaps you can explain hook_update_last_removed() better than I can?

[vstemen]

I chose to leave the code in that utilizes the php filter module for now, until we feel sure it is never going to re-surface as a contrib module. There has been a lot of discussion on the issue.

https://www.drupal.org/project/drupal/issues/1203886 https://github.com/backdrop/backdrop-issues/issues/19

I did a commit that just adds a detailed comment about it in css_injector.admin.inc.

[jenlampton]

OK. Since the database is being updated with a schema that already has those updates on the initial port, it still looks to me to be technically unnecessary. However, I can see it possibly being useful for knowing which update_N() hooks to bring across if merging any new code from the Drupal branch in the future. So I will go ahead and add the update_last_removed() call.

It sounds like you understand. The Backdrop version will contain updates assuming a specific Drupal schema version. But Drupal may continue to roll forward with more updates. Those updates will need to be duplicated in Backdrop, including specific tests to see if they are necessary (or not).

Sites upgrading from Drupal will need to be at AT LEAST the schema version number in hook_update_last_removed() so having it will prevent people from upgrading from versions that are too early.

[vstemen]

Gotcha. Thanks for the clarification. That verifies the way I thought I understood it.