Add UI for editing and removing custom (CSV) sources

Here are three options for naming/location of the tab. Content of the page Tamper can be very similar to Tamper D7.

• Manage Sources - most consistent name, where you can edit/tamper source, remove it, and maybe later add new source(?)
• Tamper and Manage Sources - longer name that points out what you can do in this tab
• Tamper Sources - old name so users of D7 know exactly where to go

I vote for Manage Sources, it's the cleanest and most consistent name. Tamper can add a link(s) there, and I could see other contrib modules using it as well, so I think it would be better to be agnostic in that respect.

Manage sources makes sense to me, but should it have "custom" in there somewhere too? Or will that tab only show up if CSV parser is selected?

@megachriz Gotcha. Makes total sense.

I suggest something more simpler and more in line with the Drupal Manage Field interface.

When user clicks edit, the row would then turn the source column to a text box (allowing users to edit the name) and the Target would turn into a dropdown (allowing users to edit as well).

OR, when user clicks edit, it brings them to another screen with allowing users to edit Source and Target and save.

Here's a summary of what we decided may be a good way to move forwards:

1) If there is a source that's not mapped (ie: a garbage source) when the user goes to the mapping screen we delete it
2) We move ahead with the UI that was proposed in #7 above
3) We should look into creating a new issue for temporary targets which existed in D7 but not in D8 in case someone needs to use a source that shouldn't be mapped anywhere

That's a good point. We should note that we need to test Feeds Tamper out to see what happens in this case.

I think that automatically removing unused sources as proposed in https://www.drupal.org/project/feeds/issues/2938505#comment-13088514 will be best solution. I do not see any use case when it will be more efficient to find unused source than to add new one. There might be a way to check programmatically if a source is used in tamper.

In this case we do not need to make any change to UI and we can keep it clean.

@MegaChriz

Do you have a patch file that can be applied so as to see those UI's in action and see how you implemented it?

1. Introduce a new plugin type called FeedsCustomSource. Downside of this is that there already is a similar named FeedsSource plugin. And they both would also kinda have the same purpose: delivering sources. Only FeedsSource would deliver predefined sources and FeedsCustomSource would deliver custom sources.

This is a perfectly sound solution. The two plugins do do two different things, ableit very similar.

2. Try if custom sources could be baked into the existing FeedsSource plugin type. The challenge here is that a single plugin now is expected to return multiple sources. So it would somehow need to be able to return custom sources too. But these custom sources are now stored in one big list on the feed type, they are not stored per source type.

This is more complicated than necessary when option 1 does the job quite well.

@MegaChriz

Do you have a patch file that can be applied so as to see those UI's in action and see how you implemented it?

Seems I was out of the loop too long and missed the move to merge requests.. and missed the merge request too.

I can't find the UI for editing/removing soгrces in latest dev?

@megachriz
Thank you, I've found temporary solution - exporting feed type via /admin/config/development/configuration/single/export , manually edit text and than reimporting back. Stay on alfa release.

Updated main description to better reflect current status.

Updating description to better reflect the work currently being done for FeedsCustomSource plugin by MegaChriz

I agree that a new plugin for custom sources is better than adding custom sources to the existing FeedsSource plugin.

@MegaChriz, could you clarify "Feeds Extensible Parsers needs to have additional fields on custom sources tab form: "Raw" and "Inner XML". see #16"
Does that mean we need an issue on Feeds Extensible Parsers to implement that there. Or do you mean these fields need to exist so that Feeds extensible parsers can make use of them?
Feeds Extensible Parsers module

One question, and some suggestions depending on the answer:

Do we need an "Add custom source" button on the custom sources tab?

It appears that we don't as these custom sources are being generated on the mapping page as per #8

This how these custom sources currently arise:

User selects a target. From the sources dropdown that appears it selects "New source..." and fills in a source.
User clicks the "Save" button to save the mapping configuration.
From the sources dropdown, user selects "New source..." again and fills in a source.
User clicks the "Save" button to save the mapping configuration.
Now the previous created source is still available in the sources dropdown and cannot be edited or deleted.

If we do need to implement an add custom source button I will remove the reference to it in this issue description.

If we do need/want to implement an Add custom sources button on the custom sources page, then I recemmend we do not do an automatic
clean up on access to the mapping page. But rather add a 'status' column to the Custom Sources tab with values of 'mapped' or 'unmapped', and leave it to the administrator to remove any unmapped sources via the operations on that source's row.

I vote to NOT have button "add custom source" - current flow with adding new custom sources in "mapping" tab works well. The primary goal for this page is allow cleanup of unused custom sources. Editing custom sources is also important, but it is less of priority for users now.

I agree with @irinaz.

"current flow with adding new custom sources in "mapping" tab works well." Yes

"The primary goal for this page is allow cleanup of unused custom sources." Yes

"Editing custom sources is also important, but it is less of priority for users now." Yes

That's enough for me to use some intiative and update that description to be concise.

I've just pulled down the code for this issue fork. My eyes are drawn to comment #16 and this screenshot.

There is no need for the type column, we only care about custom sources.

These rows should be limited to custom sources and remove the type column.

Here's a new mock up with the raw and inner html columns, and the type column removed:

@MegaChriz Thanks for hands on work you've put in to move this forward, I'm starting to get my head around the plugin requirements.

I noticed the editing implementation already in the MR, so I've added that back in to the description.

I have marked the "Create a mockup/UI design of the page" to tidy up the description.

Paraphrase the summary: "implement the UI including allowances for feeds extensible parsers integration" is basically where it's at, plus details of that.

The type column can be added or not at this point I suppose, these things can often become more apparent as the implementation progresses.

Let's go ahead and open the issue on feeds extensible parsers and add reciprocal links to both. I feel inclined to leave that in your hands @MegaChriz at least until if or when dig I dig deeper into the code and get a better understand the interplay of the sources and plugins.

Updated description to reflect that the effect on feeds tamper needs to be tested and considered.

I pushed the code I used to generate the mockup to this branch: 2938505-custom-source-mock-ui

Comparison to 2938505-custom-sources-ui: 2938505-custom-sources-ui...2938505-custom-source-mock-ui

As you can see I just cheated by directly displaying a checkbox. Having this branch gives the option for someone to tweak the mockups.

Here's a demo I whipped up to explain how I think you want these custom sources to appear in the sources drop down.

Demo screencast

I have pushed the code to make this work to it's own branch, but it's a super dirty hack, enough to demo the behaviour and that's all.
Dirty hack branch: 2938505-custom-source-plugin-parser-define-demo
Dirty hack diff (2938505-custom-source-plugin-type...2938505-custom-source-plugin-parser-define-demo)

OK.. I pushed some more changes to that 2938505-custom-source-plugin-parser-define-demo branch

This time to move the form altering logic into the CustomSource plugins..

So the concept here is the processor plugin assigned to a feedtype dictates which custom source(s) can alter the form.

A one to one relationship between customSource type and processor seems logical to me.

The Default custom source type here has started to become the "CSV" custom source type.

This code is really quite quick and dirty.. hopefully demonstrates a workable solution.

Added some more contextual information to the description

Going to pop down some of the ideas that are in my head.

An other thing that needs to happen: some custom source plugins should only be available for certain parsers. For example: the xml source should only be available when the XML Xpath parser (or a parser that extends that parser) is selected.

@MegaChriz

This looks very doable. The mapping form is already calling out to parsers to alter the form.. that's even how the "CSV" label gets on to the custom source option in your branch.. the parser alters the form.

By adding a mechanism so that the custom source plugins can register to a parser, the parser can then appropriately alter the mapping form.

In this way people can add custom sources that work with certain parsers simply by creating a new custom source plugin. The custom source itself decides which parsers it works with by registering with them.

In reference to the automatica delte of custom sources when accessing the mapping tab. This is making less and less sense as this issue develops.

There is a delete form inplemented in the existing branches on this issue. That provides a solution to deleting custom sources, and the manage of them is easy due to the custom sources tab.

An autodelete could be confusing for users. The example of not being able to add an 'add custom source' button is already raised. By not implementing the autodelete, we can implement an add source button.

I don't see the problem that the autodelete solves, I see a couple of problems that it creates.

New branch, WIP but is well on the way.

https://git.drupalcode.org/issue/feeds-2938505/-/compare/2938505-custom-...

Moves the responsibily for generating the custom source options from the MappingForm to the Parsers (ParserBase speficially).

Each parser plugin can specify what type(s) it is in it's annotation, eg xml or csv.

Each custom source plugin can specify what type of parser it will work with in it's annotation. eg xml or csv.

So, a custom source can register as compatible with csv, or xml, or csv and xml.. Parsers who's types contains xml will display custom source options for custom sources that are compatible with 'xml' parsers.

This patch works for both XML and Sitemap parsers.

It adds a 'CSV' custom source which is registered with 'csv' parser types.

It registers the xml custom source with the xml parser types.

it retains the 'custom default source' and registers that with both xml and csv parsers.

FeedsEx's Parsers will need to have the additional annotation. FeedsEx's ParseBase will need to extend Feeds' ParserBase and call the parent::mappingFormAlter from it's implementation of that method.

Feed type showing it uses a SitemapXML Parser:

Feed type with SitemapXML Parser showing XML and Default custom source options:

Feed type show use of CSV parser:

Feed type with CSV parser showing csv and default custom source options:

Feed type using a feeds_ex supplied parser:

Feed type with a feeds_ex supplied parser, showing no custom sources because the feeds_ex parsers do not register any, or alter the form.

Pushed some more changes to branch: 2938505-custom-source-plugin-type-bevan

The Default Custom Source will now appear on any parser, which means, at least for now, all parsers including feeds_ex parsers get at least the default custom source available.

Have refactored a little, including converting the ConfigurablePluginBase into a trait which allows for FeedsEx/ParserBase to extend ParserBase happily and inherit the custom source compatibility.

This should take care of

An other thing that needs to happen: some custom source plugins should only be available for certain parsers. For example: the xml source should only be available when the XML Xpath parser (or a parser that extends that parser) is selected.

I can start an issue on feeds_ex and push the changes I made there too. Will call it "Custom source compatibily" or something, and we
can do the feeds_ex work in there.

Update the description with a link to the new feeds_ex issue: https://www.drupal.org/project/feeds_ex/issues/3209655

Hopefully this helps greese the wheels a bit so we can update the feeds_ex issue as we go while this one develops.

Pushed a fix to 2938505-custom-source-plugin-type-bevan. The CustomSourceEditForm was not saving the custom source type. It does now.

@MegaChriz

re:

I'm not completely satisfied with the implementation yet, as the mapping form is making assumptions about which fields are defined on the custom source plugin

Could you elaborate on that?

I'm not seeing which parts of those mapping forms are making assumptions like that.

I'm not sure I comprehend you rregarding this either:

The plugins don't have any influence either on how the custom sources get saved on the feed type.

Maybe you mean something like the need to add a hook/event to:

 \Drupal\feeds\Entity\FeedType::addCustomSource

  /**
   * {@inheritdoc}
   */
  public function addCustomSource($name, array $source) {
    $this->custom_sources[$name] = $source;
    return $this;
  }

Though I'm not sure of the necessity of that as the custom source plugin is providing the configuration form that informs the contents of $source on the customSourceEditForm and MappingForm via `$plugin->buildConfigurationForm()`.

...

Oh and I just noticed what you might mean about the assuming fields of the custom source.
as in the mapping form assumes 'label', 'machine_name', and 'value' in `\Drupal\feeds\Form\MappingForm::processFormState`
Label and machine name both look like definite candidates for necessary fields on all custom types.

Value maybe not? That particular method has some readibility issues.

@megachriz, thanks for updates this weekend - should we also update issue summary with completed tasks?

I will try to do this testing before Thursday and report back.

I completed manual testing for remaining tasks, all four cases work as expected. This is HUGE improvement of UI.

This is huge - last blocker to do beta release!!! Thanks you, thank you, thank you!

We have our site set at 3x-dev which, admittedly not a great idea. When working on a feed after module updates, we noticed that, on the Mappings page, when selecting a source, there is no longer an option for "New Source" in the dropdown.

We are running 3x-dev on Drupal 9.3.4 with PHP 8. When I go to the "custom sources" tab, it tells me to add it through the drop-downs on the Mappings page.

Something happened that isn't being caught by updb.

Switching to Alpha 11 has fixed the issue. I just wanted you to know that it's possible this might be breaking existing feeds.