Implement interfaces and base classes for URL-based sources

The list/item stuff may very well be subsumed by this.

I'm not sure I see acquisition as requiring a separate stage from extraction - the two seem intertwined to me. But, yes, I'm thinking that this shared functionality would be represented in an interface, although we would want to share implementation as well, which suggests a base class with default implementations (or, maybe a trait).

Naming things is hard... Trying to think what this interface and corresponding base class would be named... It's intended for any file-oriented source (as opposed to row-oriented sources like SQL and CSV) and should work equally well for a local filesystem or for resources accessed via a stream wrapper (particularly http:// of course). Regarding "a particular bias towards acquisition over http", the only aspect here specific to HTTP is the ability to specify custom headers (like authentication tokens) - essential for the use case I'm working towards, and obviously @KarenS had use for it since she built it into the JSON plugin.

The "common specification of fields and IDs" (standard means to define them within source plugin configuration) actually may be more broadly useful than just the file-oriented sources I'm thinking of here, so that may be something to be put into a lower-level base class. Then on top of that we add the multiple file and linking support via an interface (yes, maybe DataAcquisitionInterface) and another layer of base class providing default implementations that both XML and JSON can leverage. I think the header support should go into here as well - when headers are provided in the configuration, they would be applied for any HTTP requests - any source of this type should be able to retrieve from HTTP as well as local files.

Still thinking through the details...

Just as an FYI, as a first pass I'm doing this in an 'Url' abstract base class alongside an UrlIterator base class here in migrate_plus, putting all the common functionality for XML and JSON sources there. I have this working with XML (whose Xml and XmlIterator classes are now very thin layers, nearly all XML-specific processing is in the XMLReader class), still applying the pattern to the JSON plugin. On the 'Url' name - I don't think that's too awful, since local filespecs can be regarded as implicit file:// URLs. Open to other suggestions, though.

The upshot is that configuring XML and JSON plugins will be nearly identical, allowing you to very easily switch between the two.

migrate.migration.wine_role_xml.yml (item_selector and selector values are xpaths):

source:
  plugin: xml
  urls: /migrate_example_advanced_position?_format=xml
  item_selector: /response/position
  fields:
    machine_name:
      label: 'Unique position identifier'
      selector: sourceid
    friendly_name:
      label: 'Position name'
      selector: name
  ids:
    machine_name:
      type: string

migrate.migration.wine_role_json.yml (item_selector is a depth level, selector is a key at that depth):

source:
  plugin: json
  urls: /migrate_example_advanced_position?_format=json
  item_selector: 1 
  fields:
    machine_name:
      label: 'Unique position identifier'
      selector: sourceid
    friendly_name:
      label: 'Position name'
      selector: name
  ids:
    machine_name:
      type: string

Next steps:

1. Get the JSON plugin working with the current pattern.
2. Figure out the most appropriate interfaces and potential breakup of the Url/UrlIterator classes.
3. Commit the changes to all three modules (migrate_plus, migrate_source_json, migrate_source_xml) as a stable platform for the remaining work
4. Implement custom headers for HTTP URLs base on the current JSON code
5. Implement the list/item pattern
6. Implement link following based on the current JSON code.

My plan was to get the JSON side working then upload the relevant patches here, in migrate_source_xml, and migrate_source_json. But my other project is heating up, not sure I'll finish the JSON work today, so maybe I should just put up what I've got so far. Or, perhaps more conveniently, push up the respective branches to the projects...

OK, I've pushed the migrate_plus work to branch refactor_source_plugins, and the migrate_source_xml work to branch refactor.

OK, got the JSON source plugin basically working and pushed to the 'refactor' branch in migrate_source_json.

So, now that much of the common functionality is in migrate_plus, let's think about how best to package it. Right now there are three key components:

1. The source plugin itself - Url is the base implementation, with Xml and Json source plugin implementations doing little beyond specifying default iterator and reader classes. The Xml plugin is handling namespaces (which really should go into the reader plugin), and the Json plugin has link and HTTP header support that will be moved to the base class in a following round. We're not very far from having Url being the source plugin itself, and we simply pass it the appropriate reader class to handle Xml vs. Json.
2. An iterator class (UrlIterator, extended as XmlIterator and JsonIterator). This intermediate iterator is primarily for iterating over multiple source urls (and would be the place for the link support), and it'd be nice to forgo the extra level, either by doing this in the source plugin directly or moving the handling into a reader base class. The extensions right now just create the reader, and there's only one argument difference requiring them to be different in that, so (if the iterator class is even necessary at all) UrlIterator should be usable without extension.
3. A reader class, which is the meat of the distinct plugins where the actual parsing and delivery of data is done. Right now these are independent classes for Xml and Json, but we certainly should have at least an interface in migrate_plus, and they can probably share some implementation as well. Maybe readers should be plugins themselves?

So, that's the horizontal organization. I'm also thinking in terms of the vertical organization - as I said before, I think handling fields and ids through source configuration is more general than this plugin, so that may belong at a lower-level base class. Actually, what I'm now thinking is that support ought to go into SourcePluginBase - fields() and getIds() would have default implementations which use the configuration, and source plugins would not need to implement them in all cases (and where a plugin had its own implementation it would be a default overridden by the configuration). But, that's not something we'll get into core in the immediate future, so my inclination is to put it here in an intermediate class and prove the concept in contrib before submitting as a core patch.

So, thoughts or suggestions?

I went ahead and factored the fields/ids support out into an intermediate class SourcePluginExtension - dirt simple!

Oh, and forgot to mention one other thing - right now the source plugin instance itself gets passed through the iterator into the reader - one task will be to pass only the specific information the reader needs through so we can decouple the classes.

The source fields should be calculated in the source via the source data.

Unfortunately, that's not how even the SQL sources work now - they have hardcoded lists of source fields, so the difference this functionality would make if it were in core is only in moving the field definitions from PHP to configuration, which I regard as a significant step forward - it at least makes it possible for front-end tools to manipulate the field configuration and documentation. Ideally, what I'd like to see is SQL sources automatically determining the list of source fields from the query, with any source configuration for fields overriding that to add appropriate labels.

But, aside from that, the primary motivation for putting the field definitions in configuration is to support sources like XML where you have to explicitly define your fields, and better to do that in configuration than in PHP, again because it is an enabler for tools.

If XML needs it, then I think it should be specific to that source, not generic.

The thing is, pretty much every non-database source will need it, so let's make sure they do it consistently by sharing it.

I'm not following your point about schema? I don't see why you'd need to declare a schema for the migration-specific field names here any more than you need to in the process configuration... That does remind me, though, that we do need schema for the configuration being introduced here.

I had expected to make the fields type: ignore as in the process configuration.

Source plugin fields() is now actually getting used! #2630720: Implement drush migrate-fields-source

Oh, and I was thinking of a more schema-friendly approach, instead of

  fields:
    machine_name:
      label: 'Unique position identifier'
      selector: sourceid
    friendly_name:
      label: 'Position name'
      selector: name

it could be

  fields:
    -
      field: machine_name
      label: 'Unique position identifier'
      selector: sourceid
    -
      field: friendly_name
      label: 'Position name'
      selector: name

More verbose for the YAML hand-crafter, but in the long run as we build out the toolset that becomes less of a concern (and this makes it a bit easier to build those tools I think).

I'm back! Was working on another project, and then my laptop suffered filesystem corruption that prevented it from completely booting - it took a few days to get a new laptop and get back into a productive state, so I'll be moving this forward in the coming days. I am so happy I was able to retrieve my unpushed changes here... One thing I should note is that I borked up a rebase (or maybe the early signs of corruption did - yeah, let's go with that;), so the current migrate_plus work is now on branch refactor_source_plugins2, not refactor_source_plugins.

@benjy: Can you explain why it's easier for tools to have a sequence rather than just straight mappings? From a PHP structure, both are fairly simple to work with?

My thinking was that this makes it easier because it would be fully described by schema, as opposed to using type: ignore. Haven't looked at your UI lately, but it felt like you were verging on something close to a general configuration editor, which would benefit from full schema. But, I'll defer to you on whether that's useful or not...

@zxaos: In the interim, this separate effort's been started. Is there any useful way to add the plugin variant I have so far? It actually runs, but since I'm new to the D8 version Migrate I'm not sure how close to your targeted approach it would be.

Have you looked at the patches involved here to see what your approach might add that we're not covering (yet)? I'm certainly welcome to contributions to this patch.

@KarensS: It seems like the CSV plugin might also use the same base? I can't tell for sure if that's the intention.

Yes, I'm thinking it could, haven't had time to look into it specifically though. We'll want to get heddn on board with that.

Also I'm open to using the same xpath selector pattern in the JSON plugin as is used in the XML plugin for more consistency. The underlying code could explode and count the values to derive the item depth.

Yes, I was thinking of that as well. The implementation would be entirely in the JSON plugin, so I see that as a follow-up issue in that queue.

So, reminding myself where I left off - I was working towards eliminating the intermediate iterator class pattern that came from the XML plugin, and making the reader classes plugins. At that point, the XML and JSON modules would just contain reader plugins, and despite my original feeling that source plugins should have their own modules I'm starting to think that we're approaching the point where the layer they're implementing is thin enough that maybe they should just be in migrate_plus after all. Thoughts?

I've pushed my latest code up to the refactor_source_plugins2 branch, plus pushed up the corresponding migrate_source_xml changes to the refactor branch over there. Don't look too closely, still very much a work-in-progress - my immediate next steps are:

1. Make the JSON plugin work with this architecture
2. Convert the reader classes to plugins (some framework for that is in place)

All right! I now have the reader plugins for XML and JSON working (at least against migrate_example_advanced), I've pushed it up to branch refactor_source_plugins2 in migrate_plus, as well as the "refactor" branches in migrate_source_xml and migrate_source_json. Moving forward from here, next steps:

1. Clean up the code, properly document the new stuff, etc. Please don't look too closely at the code yet...:P
2. Move the client/header stuff from the JSON plugin to the general URL plugin, so XML can send HTTP headers, and JSON can work from local files.

A couple of questions for interested parties (in addition to feedback on the general architecture), need to be resolved before committing the work:

1. Thoughts on moving the JSON and XML reader plugins into migrate_plus (particularly from the migrate_source_xml and migrate_source_json maintainers, who I would of course welcome as co-maintainers here)? As I said, what would be left in those modules is starting to look a little thin to be in separate modules. There would also be the short-term benefit of being less disruptive to anyone currently using those modules. But I'm not strongly wedded to the idea.
2. Thoughts on the "reader" name for this plugin? It sounds so generic, but I haven't come up with a better idea yet...

I've pushed up some cleanup changes - fixing docs, a little renaming here, a little public->protected there...

So, I got a look at the feed I'm ultimately supporting with this work today. I had been particularly interested in the HTTP header support since I understood we would need to pass an authentication token. However, as it turns out, the project has a client class that handles the HTTP request (including authentication) directly. I had already been thinking that, rather than detect HTTP connections and use the Client class only in that case, it might be cleaner to have a local file client, so a client class is always involved (and not specifically HTTP-oriented). In this case, we'll want the migration's client class to wrap the project's client class to deliver the XML or JSON payload to the reader. So, my next step is to make the client more general - and, perhaps, make it a plugin. Looking back at @mbaynton's DataAcquisitionInterface thought, perhaps this would be a data_acquisition plugin, and the current "reader" plugin would be a data_parser plugin?

"Everything is a node" used to be Drupal's mantra, then in D7 "everything is an entity" - now in D8 "everything is a plugin";).

Making some progress here... At least for the moment, my answer to the naming question is to change the "Client" class to a "data_retriever" plugin type, and the former reader plugins will become data_extractor plugins.

A bigger issue is that the current model for the data_retriever won't work for the XML source plugin as now implemented. It uses the XMLReader PHP class to incrementally parse the XML source, so large files won't absorb all available memory. So, the model where the source plugin invokes the extractor, which calls the retriever to get all the data and then creates an iterator over that data, won't fly. I'm also thinking that the iteration over multiple files logically belongs to the retriever rather than the extractor. All of which suggests the retriever rather than the extractor should be the source plugin's iterator.

Still thinking through the implications of this...

@benjy: Good suggestion, I'll rename data_retriever to data_fetcher and data_extractor to data_parser to better track Aggregator and Feeds (the "processor" stage, of course, corresponds to the core process and destination plugins).

My long-term dream is to make stuff like fetchers/parsers (not to mention Migrate's plugins) components that migrate/aggregator/feeds/etc. could all build from. Also peace on earth.

@KarenS: Thanks, I'll move the JSON class into migrate_plus. And deal with the test business when it's stable enough to update the tests...

OK, so I've used the current branch for this work with the custom API in our project that needs it, and I got it working with no further changes. I'm feeling good about the general architecture, so I think it's time to stop doing everything in a mega issue - I'll commit what we have here and open distinct follow-up issues to finish it up. That will make it easier for people to test it, provide feedback, contribute to specific follow-ups, etc. Caveat emptor - there *will* be further refactoring, so once you start working with this don't blindly update the module. You do have a little time to play with it though, I'm going off for the holidays so work on this will resume in 2016.

Slightly off-topic - I don't recall who it was that recently was asking me about migrating address fields, which I haven't previously done in D8, but I did get address field migration working in our case, where the incoming JSON has an Address array. Sample:

  # Destination subfields correspond to columns in the field table, source
  # subfields to subfields of Address in the source JSON.
  'field_event/address_line1': 'Address/Address1'
  'field_event/postal_code': 'Address/PostalCode'
  'field_event/administrative_area': 'Address/State'
  'field_event/locality': 'Address/City'
  # @todo: Translate country name to 2-character country code in prepare_row
#  'field_event/country_code': 'Address/Country'
  # Country code is required - if no value is provided, the location will
  # silently remain empty (a frustrating sticking point in working this out).
  'field_event/country_code':
    plugin: default_value
    default_value: US

Not sure why I failed to set Fixed in the first place, but given we've got follow-ups in place, finally closing this.