Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
We need an upgrade path for the D7 Aggregator module.
Remaining Tasks
Write migrations to handle the following:
- Any variables maintained by aggregator need to be moved into configuration
- Categories, feeds, and feed items need to be migrated
Aggregator's data structure in D7 seems to be very similar to what it was in D6, so it may be possible to reuse or generalize the existing code from the D6 upgrade path.
Comment | File | Size | Author |
---|---|---|---|
#19 | 2500469-19.patch | 21.73 KB | phenaproxima |
#13 | upgrade_path_for-aggregator-d7-2500469-13.patch | 25.15 KB | svendecabooter |
#10 | upgrade_path_for-aggregator-d7-2500469-10.patch | 26.03 KB | svendecabooter |
#6 | upgrade_path_for-aggregator-d7-2500469-6.patch | 26.87 KB | svendecabooter |
#4 | upgrade_path_for-aggregator-d7-2500469-4.patch | 21.55 KB | svendecabooter |
Comments
Comment #1
svendecabooterHere is a patch that migrates Aggregator feeds and feed items to Drupal 7.
Includes tests and unit tests.
The source plugin that was used for the D6 migration, has been made generic so it works for both D6 & D7.
I'm not sure if other classes can be generalised, since the D6 table {aggregator_feed} contains 1 field less than the D7 & D8 variant.
Comment #2
phenaproximaLooks pretty good after a quick scan-through, but a few relatively minor issues:
This will need to be codified in an update hook (aggregator_update_N().)
Nit: Should be
source: fid
.Two things -- first, let's make this a single return statement instead of doing a separate
return $query
; second, I'd rather we do fields('ai') instead of listing the fields individually.Same here -- fields('af') and make the entire thing a single statement.
Should be in the aggregator @group.
Nit: public static
The @var comment should have the fully-qualified name of the class or interface.
@group aggregator, and the comment should say something like "Tests migration of aggregator items."
public static
Don't do this -- just call $this->executeMigration('d7_aggregator_feed'). I'm trying to move us away from the use of prepareMigrations().
Again, the @var needs the fully qualified class name.
The inline comments should be removed.
Nit, but please format the value of $databaseContents to match other tests. Or, better yet: it looks like databaseContents and expectedResults are identical, so it's preferable to simply set
$this->databaseContents['aggregator_item'] = $this->expectedResults
in the setUp() method, rather than repeating code.Should be
$this->databaseContents['aggregator_feed'] = $this->expectedResults
. No need for the foreach.Comment #3
phenaproximaFor a difference that minor, it's acceptable to use
$this->getModuleSchemaVersion('system')
in the source plugin to check which major of Drupal is being sourced. Generalizing FTW!Comment #4
svendecabooterAttached a new version of the patch, with the comments by phenaproxima tackled, except for the first one.
I have no clue what code should go into the aggregator_update_N() hook... any pointers here?
I also made the AggregatorFeed source plugin more generic, so it can be used by both the D6 & D7 migration.
Perhaps unrelated to this patch, but I noticed that a bunch of the improvements suggested by phenaproxima (e.g. remark 6, 9, 10, 14, ...) are also in the D6 version of the aggregator migration code. Would it be within the scope of this issue to clean that up as well?
Comment #5
phenaproximaTake a look at block_content_update_8002() -- it does much the same thing. Any migrate patches which change existing migration templates need to supply update hooks because we need to support the beta-to-beta update path that was recently introduced into core.
Yes, it's in scope. Clean all the things!
Comment #6
svendecabooterUpdated patch in attachment, containing:
* update hook for beta-to-beta update path, updating the source plugin consolidation in the D6 migration templates
* code cleanup in D6 aggregator migration files, based on feedback in #2
Comment #8
svendecabooterComment #9
phenaproximaNit: s/&/and
This is too brute-force -- if the migration exists, it may have source DB credentials already embedded in it. Setting source to $template['source'] as we do here would overwrite that. If all we're doing is changing the plugin ID, let's do
set('source.plugin', 'aggregator_feed')
.Ditto this. Better to make smaller changes. In this particular instance, the change to the process pipeline is unnecessary. It's OK if
source: fid
is a list; that was just a style thing. It will work either way.$this, not $$this :)
This should stay in the migrate_drupal_6 group. It's confusing as hell, but the Drupal 7 tests stay in their module group and the Drupal 6 tests go in migrate_drupal_6 so that they will be found by MigrateDrupal6Test (of which there is no Drupal 7 version, nor will there be).
Should be in the migrate_drupal_6 group. Sorry for the misunderstanding!
Ditto.
Why are we asserting this?
Comment #10
svendecabooterFeedback taken into account in this new version of the patch.
Regarding #8: this was duplicated from the D6 version of MigrateAggregatorFeedTest. Should it be removed in both tests?
Comment #11
phenaproximaFew small things, but other than that this is squarely RTBC.
I am so, so, so sorry for backpedaling on this, but it looks like we won't actually need this update hook after all. Discussing with @benjy, @catch and @alexpott, it looks like the migration system, being experimental in 8.0.0, will not need to support update paths yet. See #2569469: [Policy, no patch] No need to provide an upgrade path for Migrations.
I don't think we need to order by fid.
Yeah, we might as well remove this. It doesn't add any value that I'm aware of. If it's in the D6 test, feel free to kick it out of there as well :)
Nit: Please add an extra line before the end of the class.
Nit: There's an extra line.
Comment #12
svendecabooterComment #13
svendecabooterUpdated patch to fix remarks by phenaproxima.
Comment #14
phenaproximaThis rocks the socks off the block. Whole-heartedly RTBC.
Comment #17
phenaproximaTestbot WTF.
Comment #18
phenaproximaI'm sorry to say...
Comment #19
phenaproximaRe-rolled.
Comment #20
phenaproximaIn the land of constant changes, the rerolled patch is king.
Comment #21
phenaproximaEr, wait -- somehow the re-rolled patch looks wrong. What the h-e-double hockey sticks?
Comment #22
phenaproximaOK, back to RTBC.
It's jarring, but the difference between #13 and the re-roll in #19 appears to be my Git behaving differently from @svendecabooter's. Git spins me right 'round. Like a record.
Comment #23
webchickCommitted and pushed to 8.0.x. Thanks!