This is part of #1775842: [meta] Convert all variables to state and/or config systems

CommentFileSizeAuthor
#17 drupal_js_cache_files-1798724-17.patch2.6 KBalbert volkman
#14 13-14-interdiff.txt801 bytesalexpott
#14 drupal_js_cache_files-1798724-14.patch3.12 KBalexpott
#13 drupal_js_cache_files-1798724-13.patch3.13 KBalbert volkman
#13 interdiff.txt525 bytesalbert volkman
#11 drupal_js_cache_files-1798724-11.patch3.13 KBalbert volkman
#11 interdiff.txt2.6 KBalbert volkman
#9 1798724-drupal_js_cache_files_4.patch3.1 KBandreiashu
#5 1798724-drupal_js_cache_files_3.patch2.56 KBandreiashu
#3 1798724-drupal_js_cache_files_2.patch2.59 KBandreiashu
#1 1798724-drupal_js_cache_files_1.patch2.59 KBandreiashu

Comments

andreiashu’s picture

andreiashu commented
Status: Active » Needs review
StatusFileSize
new 1798724-drupal_js_cache_files_1.patch2.59 KB

Let's see...

Status: Needs review » Needs work

The last submitted patch, 1798724-drupal_js_cache_files_1.patch, failed testing.

andreiashu’s picture

andreiashu commented
Status: Needs work » Needs review
StatusFileSize
new 1798724-drupal_js_cache_files_2.patch2.59 KB

Removed the double space from drupal_build_js_cache()

Status: Needs review » Needs work

The last submitted patch, 1798724-drupal_js_cache_files_2.patch, failed testing.

andreiashu’s picture

andreiashu commented
Status: Needs work » Needs review
StatusFileSize
new 1798724-drupal_js_cache_files_3.patch2.56 KB

Needed "git config core.filemode false"

andreiashu’s picture

andreiashu commented

@alexpott: I tested the '?:' operator and it works as advertised - the following code prints out default:

$val = NULL;
$new_val = $val ?: 'default';
print $new_val;
nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue tags: +JavaScript

tagging

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

Just realised that actually whilst this does not need an update function to migrate the variable we should be deleting the old variable in an update function so it is not left lying around.

I tested the patch and it works as expected.

andreiashu’s picture

andreiashu commented
Status: Needs work » Needs review
StatusFileSize
new 1798724-drupal_js_cache_files_4.patch3.1 KB

Attached new patch with the hook update. Thanks for the review!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

There's been some discussion around the namespace strategy for state key names on #1790920: Move cron_last, node_cron_last and common_test_cron to state system - the how to has been updated http://drupal.org/node/1787318

So in this instance we probably should change the key to system.drupal_js_cache_files

albert volkman’s picture

albert volkman commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt2.6 KB
new drupal_js_cache_files-1798724-11.patch3.13 KB

Variable name updated with system namespace.

Status: Needs review » Needs work

The last submitted patch, drupal_js_cache_files-1798724-11.patch, failed testing.

albert volkman’s picture

albert volkman commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt525 bytes
new drupal_js_cache_files-1798724-13.patch3.13 KB

Forgot to update system_update_N() number.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new drupal_js_cache_files-1798724-14.patch3.12 KB
new 13-14-interdiff.txt801 bytes

Patch to bump system_update_N() number and fixes documentation. Looks good to go. Thanks for the work.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ b/core/includes/common.inc
@@ -4720,7 +4720,7 @@ function drupal_add_tabledrag($table_id, $action, $relationship, $group, $subgro
-  $map = variable_get('drupal_js_cache_files', array());
+  $map = state()->get('system.drupal_js_cache_files') ?: array();

Why didn't we remove the 'drupal_' prefix in the state name?

That prefix was previously used to (poorly) namespace the variable name to a non-existing "drupal" extension. The proper extension namespace is 'system' now, so it should be removed.

albert volkman’s picture

albert volkman commented
Status: Fixed » Needs review
StatusFileSize
new drupal_js_cache_files-1798724-17.patch2.6 KB

Here's a patch for that.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Thanks!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Thanks! Committed/pushed to 8.x.

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