🇳🇱Netherlands @megachriz

megachriz → created an issue.

Nice work so far! I've some remarks:

1. I think that we should avoid <code> tags in translatable strings.
2. On the feed form, it would be useful to see what the default configured headers are, maybe as a placeholder for the textarea.
3. I think it would be better if the configured http headers are saved as an array instead of a string. For both the feed type and the feed.
4. Needs tests.

Test coverage

• Functional test for configuring a feed type with http headers:
  • Assert that misconfigured headers result into a validation error.
  • Assert that empty lines are ignored and not saved as configuration.
  • Assert that configured headers are saved.
• Functional test for configuring a feed with http headers override: test the same things as for configuring a feed type with http headers.
• Unit test for asserting the expected headers when they are configured on the feed type.
• Unit test for asserting the expected headers when they are configured on the feed.
• Unit test for asserting the expected headers when they are configured on both the feed type and the feed.

To summarize:

• You have three feed nodes using the same feed type.
• The content item that got unpublished was only imported by one feed, not by the other two.
• The content item that got unpublished was present in the source file and even updated during the import.

Is there a way for you to reproduce the issue on a clean Drupal install? Because else it would be very hard to find out what exactly is causing this bug.

@joelpittet
Good catch about the exception throwing during clean(). I think if one of the plugins throws an exception, the remaining plugins should still be called. I have adjusted this. (In practice, only the processor plugin in Feeds implements CleanableInterface, but in theory, there could exist contributed modules that implement it for the other plugins).

Also, is there any risk that one plugin’s clean() call could mutate shared state on the feed in a way that affects how later plugins behave within the same loop?

Yes, each clean() receives the same state object, so they could manipulate that. I'm not sure yet if that is a concern. At least, that behavior doesn't change with the code changes.

Wow! This looks great. In the code I could only spot that some of the code comments and names of methods could be improved. And that there are a few unrelated fixes. Ideally these should be handled in a separate issue, but I do appreciate getting these fixed too. 😀

I'm going to apply the changes on a live site that uses Feeds just to see if that results into any regressions.

Thanks for the work so far!

Very well made! I merged it.

Possible improvement for a follow-up: allow arrays as replacement patterns. The Rewrite plugin also is limited in this regard.

Let's say you have two sources called "name" and "extension" and you have the following lines in the CSV:

name,extension
foo|bar,jpg|png
baz,gif

You might want that to result into:

I assume that is what is 🐛 The rewrite plugin renames multiple values with the same (first) row Active about.

This is a great addition! I do wonder if we should fallback to strtotime() when parsing the date fails. Maybe provide that as an option? "Fallback to strtotime() if the date could not be parsed.". What do you think?

If you have applied the Tamper "String to Unix Timestamp" to the source "date" and set the date format to "d/m/Y" and then import the following CSV:

title,date
Foo,2020-09-01
Bar,01/11/2011
Qux,01-10-2011

This is the result after import (using the latest dev releases of Feeds and Feeds Tamper):

megachriz → created an issue.

Alright, so the following is a different issue for you:

title,url,status,title2
I see the issues here are:

• title2 appears at last, even though it is used on the first mapping row.
• title still appears in the template, even if it is no longer used in the mappings.

Expected CSV template should have been:

title2,url,status

Can you be more specific with the issue that you experience and provide a step by step plan with how to reproduce it?

I merged the code! This issue took a long time to resolve, because in the past years I had put most of the time into getting Feeds to a stable release.

For the most optimal experience the latest dev of Feeds is required. ✨ Allow parsers and event subscribers to mark an item as invalid Needs work has been merged as well.

When this gets into a release of Feeds Tamper I plan to create a new Feeds release on the same day.

I merged the changes!

I will merge 🐛 Catch TamperException to prevent import from crashing Needs work shortly.

Navigation Extra Tools doesn't make any difference for me. The button "Save and import" still works for me. Could it be theme related? Or are there perhaps JS errors on the page?

I also tried to reproduce the issue on Drupal 11.2.2, but I also did not encounter the reported issue with that version.

I tried to reproduce this issue on Drupal 11.1.8 and PHP 8.3.10, on a clean install, but for me the button "Save and import" works fine on the feed add page.

Steps that I took:

1. Installed Drupal using the standard profile (via drush si).
2. Enabled modules "Navigation", "Feeds" and dependencies.
3. Added a feed type with the HTTP fetcher and the RSS/Atom parser.
4. Added mappings: "Title" to "Title (title)", "Item GUID" to "Feeds item (feeds_item): Item GUID", "Description" to "Body (body): Text".
5. Added a feed with title "Feeds" and feed URL " https://www.drupal.org/project/issues/rss/feeds → ".
6. Clicked "Save and import".

The import went fine and imported 50 items.

So I think this needs more steps for reproducing the issue, try to reproduce it on a clean install.

Interesting issue. As I'm not familiar with the Trash module yet, I'm not sure what the best way is to solve this problem. If possible, I would like to avoid the soft dependency on the Trash module, because making sure the Trash specific code keeps working would require tests and a dev dependency on the Trash module. Also, I'm aiming for a pass on PHPStan. With the current implementation, I think that will mean that eventually all child classes of FieldTargetBase would need to pass the Trash service instance to the parent. See 📌 (PHPStan) Add dependency injection for FieldTargetBase and add deprecation warnings Active for other services I've planned to inject in FieldTargetBase in the future (and require them in Feeds 4.x).

One thing we could do instead, but I'm not sure if that's enough, is add a query tag to the query that Feeds executes, for example:

$query->addTag('feeds_unique_query');

This tag would then always be added.

In theory, you could then implement hook_query_TAG_alter() to perform the Trash specific code, though I don't know if adding another query tag in such implementation has the desired effect.

Closed ✨ Add support for redirect_source field types Needs review as a duplicate.

Feeds only aims to support targets for fields provided by Drupal core, so therefore I'm moving this issue to the Redirect issue queue.

There is already an issue there about it, so closing this as a duplicate as well.
✨ Feeds: redirects_source field target class. RTBC

I see that the unit tests for the Copy plugin were a bit weak. Thanks for the improvements! I merged the code.

I hope to look at your other contributions soon.

megachriz → made their first commit to this issue’s fork.

Thanks for providing code and tests! Looks good at first glance.

I think that the tests fail because of the following warning:

1 test triggered 1 PHP warning:
1) /builds/issue/tamper-3530101/vendor/twig/twig/src/Template.php:345
Array to string conversion
Triggered by:
* Drupal\Tests\tamper\Unit\Plugin\Tamper\TwigTest::testTwig
/builds/issue/tamper-3530101/tests/src/Unit/Plugin/Tamper/TwigTest.php:93

I'm kinda waiting for a new release of Role Delegation, because tests currently still fail on PHP 8.4 most likely (but not 100% sure) only because of PHP 8.4 errors in that module. 📌 Nullable types must be explicit Active was recently merged. But if a release on their side takes too long, then perhaps it would be good to create a new release that is at least partly PHP 8.4 compatible.

If you could add tests for it too, that would be great! (A unit test and a functional test are required in order for the code to be merged.)

Sounds like a good idea! Do you need any pointers to start with writing such a plugin?

A feed gets in a locked state as soon as an import for it gets started. It gets unlocked again when the import successfully finishes. When an import ends abruptly (for example because of a PHP fatal error), the feed remains locked.

The easiest way to get a feed in a locked state (and have it be in that state for a bit longer) is to go to a feed, click on "Import in background" (/feed/x/schedule-import) and click the button "Schedule import". This will lock the feed and create a queue task. The queue task will be performed on cron runs. So long that you don't cron, the feed remains locked (unless you unlock it manually).

You can also manually lock a feed using the drush command 'feeds:lock':

drush feeds:lock 1

'1' is the ID of the feed to lock.

Not that I know of. The file target in Feeds can currently only download the image from a url. So without custom code or additional modules the image would need to be downsized at the source.

I do have written an alternative file target that accepts a base64 string as input in my Feeds DEV → sandbox project. But then you would still need to write Tamper plugins to download the image, downsize it and convert it to a base64 string.

It would probably be better idea to make a change to the image target in Feeds to respect the maximum image dimension setting. Too bad that setting is not evaluated automatically when storing images programmatically.

There should be a button called "Update" on the settings form. Theming issue? Try if you can replicate the issue with a different admin theme. If you then see the update button, the issue is likely theme related.

Screenshot from a Dutch site:

I see that this feature was completed when #2989279: Add a target to entity ID → was done.

Thanks for working on this!

I think that the Expire feature should be able to work without requiring an import. So it should get triggered on cron runs by itself. Do you think that makes sense? Is that doable without loading all feed types on each cron run?

The Tamper "Unix timestamp to Date" requires indeed a number as input.

What you could do is the following:

1. Map a blank source to your date (or text) field.
2. Add the Tamper "Set value or default value" and set the value to "now".
3. Add the Tamper "String to Unix Timestamp", this will convert "now" to the current time.
4. Add the Tamper "Unix Timestamp to Date" (with for example the format "Y-m-d"). You'll get the current date.

If you set the mapping target as unique, I think you'll get one item each day.

Do note that you could possibly get duplicate items imported in case the item on day 1 is the same as on day 2.

Yes, this is possible. You just need to have a field on your content type that can be used as a unique identifier. That can be any text or number field. Using the node ID is also possible. Feeds Item GUID may not be possible, because I'm not sure if Devel Generate sets a value for that field, I would assume it does not.

On the mapping page you need to map to the field that can be used as a unique identifier and for that target, be sure to click the checkbox in the column "Unique":

This feature has recently be added to Feeds: ✨ Change RSS default importer to accomodate multiple values in Active

Currently it is only available in the dev version.

Feeds only aims to provide mapping targets for fields defined by Drupal Core. This looks like to request support for a field type by a contrib module, so the FeedsTarget plugin should be added to that module instead.

I see an issue already exist there, so closing this as a duplicate as well.
✨ Feeds mapper Active

Alright. I tested the code myself by using chmod 000 on certain directories to see if the directory got displayed in the error message on import and it did.

I merged the code.

This is now ready to be tested in combination with 🐛 Catch TamperException to prevent import from crashing Needs work for Feeds Tamper.

I looked through the code one more time, did not spot anything suspicious. Merged it. Thanks all who helped on this issue.

Upon an exception, an item can now get marked as invalid. This causes the item to fail validation and the item will not be imported.

Depends on ✨ Allow parsers and event subscribers to mark an item as invalid Needs work .

Result of applying the Tamper plugin "Filter items" while the source data is a string instead of an array:

And in the log (Feeds Log module):

@fonant
Do not that this is a patch for the D7 version of Feeds Tamper. Did you try to apply it on the D7 version of Feeds Tamper?

I'm still willing to commit code to the D7 version of this project, but I won't put much time in testing it myself.

In two places I've added the directory name to the error message. I think that's all, or did I miss one?

megachriz → made their first commit to this issue’s fork.

Thanks for testing! I merged the code.

@max.valetov
I did not take a deep dive into the Media target issue yet, mostly only worked on the tests so far.
I do know that the source that you should map to the Media target must be an url using the http or https scheme.
Other source types, like local path or not yet supported, but there is being worked on: ✨ File target: add support for local file path as source Needs work .
The source url must contain an extension as well, but there is an issue open for supporting file urls that do not specify the extension: 💬 Failed to validate remote image with no file extension Needs work

If you are not sure what value Feeds receives, I recommend to enable the Feeds Log module (included with Feeds). With that module you can see what items were processed and what the source item looked like at the time of processing.

If you can write a test that demonstrates the bug, that would be great! The test should be added to \Drupal\Tests\tamper\Unit\Plugin\Tamper\ConvertBooleanTest.

Since there is a patch, I set the issue status to "Needs work". "active" indicates that there's only a report and no existing code.

I've added a possible fix, still needs work because a new dependency called 'plugin.manager.field.field_type' is not injected yet. It might not be possible yet to inject it without either a BC break or a deprecation warning.

No fix yet, but I made a start with test coverage.

megachriz → made their first commit to this issue’s fork.

@thomasmurphy
I have not tested it, but I think that one, two, three doesn't work because there are spaces used. So the column names effectively became "one", " two" and " three". " two" is not equal to "two", because the first one starts with a space and the second doesn't.

In 🐛 CSV headers with extra space don't get mapped to correct fields Needs work there is being worked on a feature to ignore leading or closing spaces in CSV column names.
There also exists an issue to implement a feature to detect mismatches in configured CSV column names vs actual CSV column names: ✨ Allow parsers to validate source before import Active .

I've added a source called 'authors'. It returns an array. If you want to format this as a comma-seperated string, use the Feeds Tamper plugin "Implode".

megachriz → made their first commit to this issue’s fork.

Thanks for reporting back! I'll mark this issue as fixed then. The dependency implementation that should help to prevent this issue can be handled in 🐛 Field feeds_item is unknown when looking up a taxonomy item that doesn't exist Active .

No, importing from relative paths is not supported yet. Only urls using the http or https scheme are supported for importing files at the moment.

Adding support for relative paths is high on the wishlist, there is being worked on in ✨ File target: add support for local file path as source Needs work .

Thanks for filing a bug report. Perhaps the bug is that the plugin has set 'handle_multiples' to 'TRUE' in the annotation. Can you check that when you remove that if that also fixes the problem?

@dudeweb
This module does get very little attention from me, so indeed also for the docs. Great that you were able to figure out how to get products imported. It is indeed product variations first and products second. I don't know of an easier way.

If you run into issues like these, it's okay to ask a question on Drupal Slack → . For Feeds specific questions, there is the #feeds channel. You are not guaranteed to always get an answer quickly, but the community is quite active on Slack, so it's worth a shot. 🙂

Did you try to reproduce the issue on a clean install? Because when I import two items with the same exact file (with the same name), then I get as result that both items reference the same file entity. In your case, you get an error. So there must exist a difference between my setup and your setup. The most likely difference is that you:

• Have a module installed that I have not.
• And/or have configured a file related setting differently.

To find out what that difference is, try to replicate the error on a clean install so we know which module or setting causes a different behavior.

Based on the current info, the only possible solution that I see is making sure that file names are unique. But there could perhaps exist an other solution if we can find out what causes a different behavior.

Can it be that some records reference text files that are in a different directory, but with the same name?

The error message says that a file from the temporary directory could not be written to either the public or private file system because on that file system there already exists a file with the same name.

I'm unable to reproduce the error however. Even when I try to import a CSV file like the following, I don't get an error:

guid,title,file
1,A,http://example.com/feeds/txt/a/sample.txt
2,B,http://example.com/feeds/txt/b/sample.txt

Or this:

guid,title,file
1,A,http://example.com/sites/default/files/bijlagen/sample.txt
2,B,http://example.com/sites/default/files/bijlagen/sample.txt

Can you try to reproduce the issue on a clean install? Maybe a file related module is configured in a way that causes this issue.

Using private:// or public:// as file source is indeed not yet supported. Only http sources are supported at the moment. There is a request to support the local file path a source: ✨ File target: add support for local file path as source Needs work .

Perhaps that resaving the feed type will fix the problem for you - if the cause is indeed a missing feeds_item field.

Can it be that you accidentally deleted the feeds_item field? Or did you not deploy the field? This field gets created automatically when you save a feed type. It holds metadata about the import, such as when an item was imported last.

There is a related issue about this where I propose to create a dependency on the feeds_item field on the feed type, so that the issue can no longer occur on deployment.
🐛 Field feeds_item is unknown when looking up a taxonomy item that doesn't exist Active

I'm trying to figure out from your bug report what you are trying to do and what problem you actually face. It seems related to importing data into a file field on a node.

Can you explain further with CSV examples what you are importing and at which moment something goes wrong and what goes wrong exactly?

For example:

1. Create a feed type using the CSV parser with mapping to Feeds Item Guid, title and a file field. Set Feeds Item guid as unique.
2. Import the following CSV file:
   

   id,title,file
   1,Foo,http://www.example.com/file1.txt
   2,Bar,
   

   If I read correctly, this import goes fine. New records with a text file reference and new records without text a file reference are getting imported.

3. Change the CSV file and import again.
   

   id,title,file
   1,Foo Updated,http://www.example.com/file1.txt
   2,Bar Updated,
   

Record 2 updates fine, but for record 1 you get an error? Or do you get an error when on an update you reference a different text file? Or do you get an error if on an update the text file doesn't exist on the source?

I see that RNG has no gitlab CI support enabled yet. Do you want to add that, perhaps in an other issue, and then fix any test failures that may occur?

If you want to make a start on this, that would be great! It will take a while until the site that I use RNG on is ready for Drupal 11, so that's why I gave porting this module to D11 a low priority.

I see that the new properties are also exported when calling BaseItem::toArray(), which I think we don't want. So these properties should not be exported. So that should get fixed, but perhaps before we do that it makes sense to finalize 🐛 Creation of dynamic property Drupal\feeds\Feeds\Item\SyndicationItem::$parent:fid is deprecated Needs work first, because in that issue BaseItem::toArray() gets also changed. And I think that issue is good to go, just waiting a few more days for possible feedback on that one.

For backwards compatibility I have moved the new methods to a new interface called ValidatableItemInterface. Feeds Tamper can then check if the item implements that interface before trying to call the new methods. This way, Feeds can still be compatible with older Feeds versions.

Let's see if the test passes now.

I tried to reproduce the issue using the following steps:

1. Created a taxonomy called 'tags'.
2. Created a content type called 'article' with a taxonomy reference field for the taxonomy 'tags' and allowing an unlimited number of values.
3. Created a feed type using the CSV parser and the node processor with content type 'Article'.
4. Add mappings to 'Title' and 'Tags'. Configured the mapping target 'Tags' to reference by name and to autocreate terms in the vocabulary 'tags'.
5. Added a Tamper Explode plugin to the 'Tags' mapping target and using | as delimiter.
6. Imported the following CSV file:
   

   title,tags
   Lorem,Lehigh|Activities
   Ipsum,Lehigh|Residence Camp
   

My result was that two articles got imported, both with two terms.

Can you share your configuration and provide a sample source file? This way I can probably quicker try to reproduce the issue. One way to share configuration is by packing it inside a feature → module.

Or else can you provide the exact steps to reproduce the issue?

Thanks for creating the issue. At first I could not reproduce it. I created several feed types and for each one the CSV template matched the order of the mappings.

But then I decided to edit the mappings of a feed type and created a new CSV source for the first mapping.

First the mappings where this:

And that resulted into the following CSV template:

title,url,status

Then I mapping a source called "title2" to "Title (title)", and I got the following CSV template:

title,url,status,title2

I see the issues here are:

• title2 appears at last, even though it is used on the first mapping row.
• title still appears in the template, even if it is no longer used in the mappings.

Expected CSV template should have been:

title2,url,status

Am I correct that this is what the issue is about?

I also sense a request here to be able to change the order of mappings. That should be handled in a different issue. Or is it the order of custom sources?

I adjusted the code a bit, because I did not like that 'data' became a preserved key. The changes ensure that a field called 'data' can be set without overwriting the complete $data property.

I also expanded the test coverage a bit.

Thanks for your contribution! I merged the code.

The Feeds Tamper logo is added to the project. :)

The Tamper logo is added to the project. :)

@baikho
Can you provide a test that demonstrates the bug? This way we can ensure that the bug doesn't accidentally return in the future. The test case should be added to \Drupal\Tests\tamper\Functional\Plugin\Tamper\EntityFinderTest.

Note: I moved the code to a new branch called '3517986-entity-finder-fieldable', because I had some trouble pushing/pulling from/to 8.x-1.x, which is a branch name already used in the main repo.

megachriz → changed the visibility of the branch 8.x-1.x to hidden.

megachriz → changed the visibility of the branch 3517986-entity-finder-plugin to hidden.

megachriz → made their first commit to this issue’s fork.

I think that we have enough tests to start with.

It took some effort, but all tests are passing again! Merged.

megachriz → created an issue.

I made a start with writing the tests. The tests are currently failing though.

megachriz → changed the visibility of the branch 2928904-add-a-mapping to hidden.

megachriz → changed the visibility of the branch 2928904-add-a-mapping-test to hidden.

No more cspell errors! Merged.

megachriz → created an issue.

I merged the changes!

megachriz → created an issue.

Merged.

megachriz → created an issue.

I experienced this issue too when working on tests in 🌱 [markdown] Add Tests Needs work . In the test I've used the following workaround:

$config = $this->parser->getConfiguration();
$config['render_strategy']['type'] = RenderStrategyInterface::NONE;
$this->parser->setConfiguration($config);

I think that the default config should be loaded as soon as using \Drupal::service('markdown') or, as the test uses, \Drupal::service('plugin.manager.markdown.parser')->createInstance('commonmark') and not at the moment getRenderStrategy() is called. So therefore I move the status of this issue to "Needs work".

I experience that a cache rebuild is enough to make the parser available. I think that that behavior is okay, though it would be cool if that is mentioned on /admin/config/content/markdown as also said in #3259945: Help text at /admin/config/content/markdown should say that a cache clear is needed to pick up new parsers → .

Shall we close this one as "works as designed"? Or does a cache rebuild not resolve the issue for you?

I can no longer reproduce this issue. Maybe it has been fixed in the mean time. In 🌱 [markdown] Add Tests Needs work I added test coverage that follows the steps listed in the issue summary and the test passes.

Feel free to reopen if this is still an issue.

I've written a few tests to render a text with each markdown parser. Only for the CommonMark PECL parser I marked the test as incomplete because that one requires a PECL extension. Without the markTestIncomplete() call, the fails with the error Call to undefined function CommonMark\Parse().

The proposed Feeds Tamper logo in 📌 Create logo for Feeds Tamper for Project Browser Initiative Active :

I've updated the Feeds Tamper logo design by using the purple design of the Tamper logo and I made the basket purple accordingly:

The proposed Tamper logo in 📌 Project Browser: Create logo for Tamper Needs review :

I still like the purple variant of the Tamper logo a bit better, but I did make the circle a bit bigger and greener. In the previous version I think that the green was a bit to light. The color looked less well in the Feeds Tamper logo.

I committed the logo as logo.png on the project.

I reverted a commit. I think it would also be a good idea to revert anything that tries to fix deprecations from Markdown itself. However, if fixing these deprecations is necessary for Drupal 11 support, they should of course be done, but even in that case I think it would still be better to handle these in an other issue first. Because it looks like that some attempts to fix these deprecations here introduced new bugs.

I got a fatal error on a D10 site:

Error: Call to undefined method League\CommonMark\CommonMarkConverter::convert() in Drupal\markdown\Plugin\Markdown\CommonMark\CommonMark->convertToHtml() (line 183 of /Users/youri/Websites/drupal/modules/wip/markdown/src/Plugin/Markdown/CommonMark/CommonMark.php).

Is it possible that the code drops support for CommonMark v1? And is that necessary? I would say that should be handled in ✨ Add support for Commonmark v2 Active . So I think that commit 3283349 should be reverted.

GitLab CI has been enabled, though only for D10 testing for now. D11 testing should be enabled in 📌 Automated Drupal 11 compatibility fixes for markdown Active .

I've added test coverage. However, in order to make InstallableLibrary unit testable, I had to make some more changes to that class. The class lacks dependency injection.

There is a BC break in HttpClientTrait. That's not great, but is a consequence of not using dependency injection.

megachriz → made their first commit to this issue’s fork.