Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

damiankloip’s picture

Assigned: Unassigned » damiankloip

Rolling now

damiankloip’s picture

Assigned: damiankloip » Unassigned
Status: Active » Postponed

Change of plan, let's postpone on the refrenced issue in #1

damiankloip’s picture

Title: Add Drupal::state() to replace state() » Use Drupal::state() to replace state()
damiankloip’s picture

FileSize
197.54 KB
Berdir’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -907,7 +907,7 @@ function drupal_get_filename($type, $name, $filename = NULL) {
-        $file_list = state()->get('system.' . $type . '.files');
+        $file_list = \Drupal::state()->get('system.' . $type . '.files');

It's not necessary to use \ in non-namespaced code.

+++ b/core/includes/update.incundefined
@@ -1483,7 +1483,7 @@ function update_config_manifest_add($config_prefix, array $ids) {
  * Provides a generalized method to migrate variables from 7.x to 8.x's
- * state() system.
+ * \Drupal::state() system.

+++ b/core/modules/system/system.installundefined
@@ -734,7 +734,7 @@ function system_schema() {
-    'description' => 'Generic key-value storage table. See state() for an example.',
+    'description' => 'Generic key-value storage table. See \Drupal::state() for an example.',

I don't think this should talk about the function/method at all, it should IMHO just be "state system". Neither the function nor the method is a system, whatever's behind is.

+++ b/core/includes/utility.incundefined
@@ -49,7 +49,7 @@ function drupal_var_export($var, $prefix = '') {
-    // magic method __set_state() leaving the export broken. This
+    // magic method __set_\Drupal::state() leaving the export broken. This

+++ b/core/misc/ckeditor/ckeditor.jsundefined
@@ -751,7 +751,7 @@ j.style.cssText||"";if(!e)j.style.cssText="position: static; overflow: visible";
-setTimeout(function(){m.setStyle("display","block")},0)}b.fire("resize")}this.toggleState();if(q=this.uiItems[0]){p=this.state==CKEDITOR.TRISTATE_OFF?i.maximize.maximize:i.maximize.minimize;q=CKEDITOR.document.getById(q._.id);q.getChild(1).setHtml(p);q.setAttribute("title",p);q.setAttribute("href",'javascript:void("'+p+'");')}if(b.mode=="wysiwyg")if(k){CKEDITOR.env.gecko&&h(b);b.getSelection().selectRanges(k);(r=b.getSelection().getStartElement())&&r.scrollIntoView(true)}else j.$.scrollTo(l.x,l.y);
+setTimeout(function(){m.setStyle("display","block")},0)}b.fire("resize")}this.toggle\Drupal::state();if(q=this.uiItems[0]){p=this.state==CKEDITOR.TRISTATE_OFF?i.maximize.maximize:i.maximize.minimize;q=CKEDITOR.document.getById(q._.id);q.getChild(1).setHtml(p);q.setAttribute("title",p);q.setAttribute("href",'javascript:void("'+p+'");')}if(b.mode=="wysiwyg")if(k){CKEDITOR.env.gecko&&h(b);b.getSelection().selectRanges(k);(r=b.getSelection().getStartElement())&&r.scrollIntoView(true)}else j.$.scrollTo(l.x,l.y);

+++ b/core/misc/jquery.form.jsundefined
@@ -433,7 +433,7 @@
-        function checkState() {
+        function check\Drupal::state() {

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/Display.phpundefined
@@ -29,7 +29,7 @@ public function getFormKey() {
-   * Overrides \Drupal\views_ui\Form\Ajax\ViewsFormBase::getFormState().
+   * Overrides \Drupal\views_ui\Form\Ajax\ViewsFormBase::getForm\Drupal::state().

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/Form/Ajax/ViewsFormBase.phpundefined
@@ -57,7 +57,7 @@ protected function setType($type) {
-   * Implements \Drupal\views_ui\Form\Ajax\ViewsFormInterface::getFormState().
+   * Implements \Drupal\views_ui\Form\Ajax\ViewsFormInterface::getForm\Drupal::state().

;)

xjm’s picture

#6 is exactly why we should split up the config() patch. :)

damiankloip’s picture

FileSize
84.81 KB
182.45 KB

Sorry, that was silly, just use a regex to do the replacement! :)

xjm, not sure what you mean, but yes, splitting things up is good. one issue per Drupal helper function.

Berdir’s picture

@damiankloip: That was in reference to our discussion in #1957142: Replace config() with Drupal::config(). We agree that it should be at least an issue per function but @xjm is suggesting to split it up even more.

And, I'm not convinced by that by #6 :) Most of the things that I pointed out would have been syntax errors and changes in .js files are also obviously unrelated. Anyway, let's get the main issue in and then we'll see how easy this one will go on for example :)

xjm’s picture

lol @ 84K interdiff

I'll stop abusing this issue and discuss over in #1957142: Replace config() with Drupal::config()...

xjm’s picture

+++ b/core/modules/field/field.form.incundefined
@@ -212,10 +212,10 @@ function field_form_get_state($parents, $field_name, $langcode, &$form_state) {
- *   The array of data to store. See field_form_get_state() for the structure
+ *   The array of data to store. See field_form_get_Drupal::state() for the structure
...
- * @see field_form_get_state()
+ * @see field_form_get_Drupal::state()

Well, this patch should either be broken up, or use a better regex. ;)

damiankloip’s picture

FileSize
2.34 KB
180.11 KB

I don't think you can treat my lazy patch rolling as a good reason for splitting this into lots of issues. I will move that dicussion over there though :)

damiankloip’s picture

Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 1947720-12.patch, failed testing.

Berdir’s picture

Ah, looks like DUBT is missing the definition for the state service and it worked so far because state() still accessed the keyvalue service. My state cache issue has the fix for this too.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
800 bytes
180.4 KB

Yep, you're right :) This should fix it...

Status: Needs review » Needs work

The last submitted patch, 1947720-16.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

#16: 1947720-16.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1947720-16.patch, failed testing.

damiankloip’s picture

Not sure what is going on here, locally it is fine.

Berdir’s picture

I can reproduce this with run-tests.sh, haven't looked into it.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
179.83 KB

Just a reroll.

Status: Needs review » Needs work

The last submitted patch, 1947720-22.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
579 bytes
180.19 KB

There was a state() call left from the field entity patch that went in.

I think we still have the AddFeedTest issue though, not sure what is going on with that still...

Status: Needs review » Needs work

The last submitted patch, 1947720-24.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
180.12 KB

Let's reroll, and see if it's still failing.

Status: Needs review » Needs work

The last submitted patch, 1947720-26.patch, failed testing.

damiankloip’s picture

I'll reroll, that was just a git reroll. There are a few usages of state() being used since this patch was last properly rolled.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
185.97 KB

Rerolled, still not sure wtf is going on with the drupal_add_feed tests. Something on webTestBase?

Status: Needs review » Needs work

The last submitted patch, 1947720-29.patch, failed testing.

Berdir’s picture

+++ b/core/modules/system/system.installundefined
@@ -1,4 +1,4 @@
-<?php
+#<?php

This is why :)

Berdir’s picture

+++ b/core/modules/system/system.installundefined
@@ -1,4 +1,4 @@
-<?php
+#<?php

This is why :)

damiankloip’s picture

FileSize
303 bytes
185.87 KB

Haha, what a great interdiff.

Berdir’s picture

Status: Needs work » Needs review

.

Berdir’s picture

... double post.

Status: Needs review » Needs work

The last submitted patch, 1947720-33.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
187.06 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, 1947720-37.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
187.07 KB

Please!

Status: Needs review » Needs work

The last submitted patch, 1947720-39.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
183.47 KB

Some leftover views changes crept in. This SHOULD be good.

Status: Needs review » Needs work

The last submitted patch, 1947720-41.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#41: 1947720-41.patch queued for re-testing.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yup. Let's get this in. Almost 200K aren't that cool to review...

catch’s picture

#41: 1947720-41.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1947720-41.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
168.82 KB

Rerolled. Can you we this is asap please - It's a pain to reroll!

catch’s picture

Status: Needs review » Reviewed & tested by the community

Putting this back into the RTBC queue so it's ready to go once the bot comes back green.

damiankloip’s picture

Great- thank you!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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