while testing the upgrade path from aggregator categories to entity reference field for #15266: Replace aggregator category system with taxonomy i found out that
aggregator_update_8000 has the wrong name:)
see patch below

CommentFileSizeAuthor
#15 drupal-aggregator_update-1958702-13.patch2.69 KBParisLiakos
#11 drupal-aggregator_update-1958702-11.patch2.69 KBParisLiakos
#7 drupal-aggregator_update-1958702-7-fail.patch1.38 KBParisLiakos
#7 drupal-aggregator_update-1958702-7.patch1.9 KBParisLiakos
#1 drupal-aggregator_update-1958702-1.patch530 bytesParisLiakos
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Active » Needs review
FileSize
drupal-aggregator_update-1958702-1.patch530 bytes
ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Title: Aggregato variables to config update is never executed » Aggregator variables to config update is never executed
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Sounds like we should add those to the config upgrade path test class?

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Issue tags: +Configuration system

trying to find where the test class for variable to config is, but i only find ConfigUpgradeTest which tests fake variables.
do we have any tests on real variables?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Not sure if I understand what you mean with fake variables. Those variables are as real as you make them, the test class just contains what to assert, you need to add them to drupal-7.system.database.php so that they actually exist before the update is run.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

ah i see..with fake, i mean variables not used by real drupal sites:)
in that case i dont think we should add them there, if we add aggregator variables we should include all modules variables:/

Edit: i was looking the wrong test...SystemUpgradePathTest is the correct one

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
FileSize
drupal-aggregator_update-1958702-7.patch1.9 KB
drupal-aggregator_update-1958702-7-fail.patch1.38 KB

sth like this i guess

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

While you're at it, can you add the other variables as well?

Status: Needs review » Needs work

The last submitted patch, drupal-aggregator_update-1958702-7.patch, failed testing.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

yeah..i am supposed to serialize it first -.-

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs work » Needs review
FileSize
drupal-aggregator_update-1958702-11.patch2.69 KB

lets hope i did them correct:p

Status: Needs review » Needs work

The last submitted patch, drupal-aggregator_update-1958702-11.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Reviewed & tested by the community

This looks good if it passes.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Reviewed & tested by the community » Needs work

Hah, I guess not then.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs work » Needs review
FileSize
drupal-aggregator_update-1958702-13.patch2.69 KB

yeah alright missed a zero:P

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work 
+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.phpundefined
@@ -159,6 +159,16 @@ public function testVariableUpgrade() {
+      'items.teaser_length' => 6000,

+++ b/core/modules/system/tests/upgrade/drupal-7.system.database.phpundefined
@@ -135,6 +135,34 @@
+    'value' => 'i:600;',

6000 vs. 600.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

This now looks good for real :)

twistor’s picture

twistor CreditAttribution: twistor commented

Should aggregator_category_selector be in there?

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

no please, aggregator_category_selector is gonna die here #15266: Replace aggregator category system with taxonomy lets not make patch there even bigger:)

twistor’s picture

twistor CreditAttribution: twistor commented

Didn't even think about, just did a search for all aggregator variables. Good to go.

catch’s picture

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

aggregrator, whoops.

Committed/pushed to 8.x, thanks!

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