Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fastangel’s picture

Title: Variable to config: image.settings » Variable to config: image.settings [d7]
Status: Active » Postponed

I think that this issue should be postponed to D7. Right now the variables in D8:

 preview_image: core/modules/image/sample.png
 allow_insecure_derivatives: false
 suppress_itok_output: false

Itok integration was introduced in D7 see this for more information https://drupal.org/node/1934498
About insecure derivations was added in D7 7.20 see release note: https://drupal.org/drupal-7.20-release-notes
And the end the module in D6 isn't use any variable.

eliza411’s picture

Project: IMP » Drupal core
Version: » 8.x-dev
Component: Code » migration system

Moving to the core queue to consolidate issues now that we're doing all the work there.

eliza411’s picture

Assigned: chx » Unassigned

This one didn't need to be assigned

eliza411’s picture

Project: Drupal core » IMP
Version: 8.x-dev »
Component: migration system » Code
Issue summary: View changes
Status: Postponed » Active
Parent issue: #2125745: [meta] Variables to config migration [D6] » #2181257: [meta] Variables to config migration [d7]
samhassell’s picture

Assigned: Unassigned » samhassell
samhassell’s picture

preview_image doesn't exist in d7 so no need to migrate it. These other two allow_insecure_derivatives and suppress_itok_output are straight forward boolean mappings so no complexity there I don't think.

Couldn't get the test suite working on my local so submitting for testbot.

samhassell’s picture

Status: Active » Needs review

tests now passing.

benjy’s picture

Status: Needs review » Fixed

Committed to the drupal7 branch, fixed a small formatting issue on the way in.

Status: Fixed » Closed (fixed)

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

jcost’s picture

Project: IMP » Drupal core
Version: » 8.0.x-dev
Component: Code » migration system
Status: Closed (fixed) » Needs work

Will need to be submitted again to Core since moving from sandbox.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
10.04 KB

Rerolled against 8.0.x, with a test of the d7_image_settings migration.

phenaproxima’s picture

FileSize
9.96 KB

Slight cleanup of the test.

Status: Needs review » Needs work

The last submitted patch, 13: 2130283-13.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Postponed
phenaproxima’s picture

Status: Postponed » Needs review
FileSize
9.9 KB
phenaproxima’s picture

Needs to be merged into the parent issue.

phenaproxima’s picture

Status: Needs review » Needs work

Needs to be re-rolled with fewer alterations to the {variable} dump.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
3 KB

We be re-rollin'. Testbot, don't be hatin'.

phenaproxima’s picture

Re-rolled again. No interdiff due to merge conflict with Variable.php dump.

mikeryan’s picture

This looks straight-forward enough - but, despite what #7 says, in the D7 code base I see

  $sample_image = variable_get('image_style_preview_image', drupal_get_path('module', 'image') . '/sample.png');

I see absolutely no other reference to that variable in my D7 code base. I do see it included in #469798: Document variables without an UI. Should we migrate it?

phenaproxima’s picture

Status: Needs review » Needs work

If it exists in core, even for a single usage, I think we should migrate it.

quietone’s picture

Status: Needs work » Needs review
FileSize
2.76 KB
2.07 KB

Added image_style_preview_image.

phenaproxima queued 23: 2130283-23.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 23: 2130283-23.patch, failed testing.

phenaproxima’s picture

Issue tags: +Needs reroll
quietone’s picture

Status: Needs work » Needs review
FileSize
2.76 KB

Reroll.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

Almost there.

  1. +++ b/core/modules/image/src/Tests/Migrate/d7/MigrateImageSettingsTest.php
    @@ -0,0 +1,42 @@
    +    $this->installConfig(static::$modules);
    

    We might not actually need this line; if the test still passes without it, kill it. :)

  2. +++ b/core/modules/image/src/Tests/Migrate/d7/MigrateImageSettingsTest.php
    @@ -0,0 +1,42 @@
    +    $this->loadDumps(['Variable.php']);
    

    This should be removed.

  3. +++ b/core/modules/image/src/Tests/Migrate/d7/MigrateImageSettingsTest.php
    @@ -0,0 +1,42 @@
    +    $settings = \Drupal::config('image.settings')->get();
    

    $this->config()

  4. +++ b/core/modules/image/src/Tests/Migrate/d7/MigrateImageSettingsTest.php
    @@ -0,0 +1,42 @@
    +    $this->assertTrue($settings['allow_insecure_derivatives']);
    +    $this->assertTrue($settings['suppress_itok_output']);
    +    $this->assertIdentical("core/modules/image/testsample.png",$settings['preview_image']);
    

    Let's use $config->get() here, for consistency.

quietone’s picture

Status: Needs work » Needs review
FileSize
2.68 KB

Better?

The last submitted patch, 27: 2130283-27.patch, failed testing.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Perfect.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed d66acd1 on 8.0.x
    Issue #2130283 by phenaproxima, quietone, samhassell: Variable to config...

Status: Fixed » Closed (fixed)

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