🇳🇱Netherlands @megachriz

@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.

In 3463119-abort-validation I've added code to abort validation of the Markdown settings and also remove the Markdown settings from the form state when the markdown filter is not enabled. In my case, this allows a text format with CKEditor enabled but with Markdown disabled to be saved again.

It does not solve the underlying error, because the reported error message is still there when you both enable CKEditor and Markdown. But it looks like they aren't meant to be enabled together.

This should fix 🐛 Unable to save text format without enabling Markdown filter Active as well.

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

Good idea indeed. I merged the change.

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

I experience this issue when I haven't any markdown libraries installed. When I have the library "league/commonmark:^1.6" installed, I experience 🐛 Error when saving text format - configuration property id doesn't exist Active instead.

I've implemented a different approach in 3470570-remove-non-config-keys-from-form-state, because I'm not sure if it is correct that ParserConfigurationForm should be assumed to only be used in Filter form context. So instead, I tried to fix it within the FilterMarkdown class.

Also added test coverage that was meant for 🐛 Error when saving text format - configuration property id doesn't exist Active , but that test passes as long as "league/commonmark" is not installed, apparently.

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

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

I was able to reproduce the error this way:

1. On a text format, set the Pathologic filter after the Markdown filter
2. Try to render a text using the configured text format.

I added an unit test (oof) and I merged the changes!

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

I applied for co-maintainership and I merged the changes! Thanks all!

Additionally, I credited "dan kolbas" who had worked on a fix too in 🐛 SubformState contains 2 abstract methods Closed: duplicate (as requested by @tyler36 in #6).

The only thing I can see on the screenshot is that it says it is operating in maintenance mode, but I doubt that's the cause: I tried to import a RSS feed while in maintenance mode and that worked fine for me. I also do not think that this issue is caused by certain Feeds configuration.

When I search on the web for the given error, it hints me that it might be related to apache configuration.
https://stackoverflow.com/questions/10873295/error-message-forbidden-you...

The only other thing I can think of is that Feeds may not be able to write or read from certain directories, like private://feeds/in_progress.

We could perhaps also delete SubformState and SubformStateInterface from the BcSupport folder, because it says it was supposed to be removed from Markdown 3.0. But to not delay this issue on that, we could also handle that in a follow-up.

I've added basic test coverage. To demonstrate that the tests fail without the fix, I've added a separate MR that should not be merged.

Note: there does exist a GitLab CI feature for running tests only, but I think that feature works only with the current Drupal version (which is Drupal 11) and Markdown isn't compatible with Drupal 11 yet. So therefore I created a separate MR for it.

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

I experience this issue too. But with error reporting on, I get the following error:

Fatal error: Class Drupal\markdown\BcSupport\SubformState contains 2 abstract methods and must therefore be declared abstract or implement the remaining methods (Drupal\Core\Form\FormStateInterface::setIgnoreDestination, Drupal\Core\Form\FormStateInterface::getIgnoreDestination) in /web/modules/contrib/markdown/src/BcSupport/SubformState.php on line 16

For that error, there already is an other issue: 🐛 Subformstate incorrect interface error Active
So assume that this issue is a duplicate of that one.

@poakpong
Feel free to reopen this issue if the reason that you saw a blank page was caused by a different error. (Check the logs on /admin/reports/dblog after you faced the blank page or enable error reporting on /admin/config/development/logging.)

I agree that this is useful. On a site that I use Commerce Abandoned Carts on, I've embedded a table in the CAC email template. That table consists of two columns: the left column contains product images, the right column the product names with a short description.

For each product, I use a custom view mode that I called 'mail'. A template for a certain product type looks like this in my case:

<tr valign="top">
  <td>{{ product.field_product_image }}</td>
  <td>
    <h2>{{ product.title }}</h2>
    {{ product.field_auteurs }}
  </td>
</tr>

I think that how I implemented displaying products in the CAC mail is a bit to fancy to use as a default, as it would require a lot of setup. We could indeed go for a simpler approach as default, like you propose in the issue summary.

I see that this issue is addressed in 📌 Automated Drupal 11 compatibility fixes for markdown Active , so closing this as a duplicate.

See this line from MR !37:
https://git.drupalcode.org/project/markdown/-/merge_requests/37/diffs?fi...

@finex
I did not manually test the 2.1.x version on Drupal 11 yet, while the dev version says it is Drupal 11 compatible. See latest comment from 📌 Add Drupal 11 support for 2.1.x branch Needs review . This the main reason that I did not create a release yet.

Additionally, in #2993374-9: Use queue for sending emails → there were some suggestions for improvements that would be nice to haves for 2.1.x. I had suggested to open new issues for these before closing the issue. Not for every suggestion a new issue was created yet.

I also liked ✨ Link to the cart in the email could be non-session dependent Needs review to get in 2.1.x, but that one is also a nice to have.

@bkosborne
Thanks for your contribution. This could be a viable solution too, though it is possible that it will no longer be supported in a future version of PHP (PHP 9?). I'm not sure yet though if it is going to be removed, it's not clear from https://www.php.net/manual/en/language.oop5.properties.php#language.oop5...

Meanwhile, I've updated the other MR:

• I've addressed my own remarks from #11.
• I made sure that the data property cannot be get or set directly. An UnexpectedValueException will be thrown when trying to do so. Added test coverage for this change.

Thanks for your contribution! It looks like a valuable addition to me.

Do you want to add test coverage for it too? This way we can ensure that no mails are being sent when the option is enabled.

I'm also thinking if it would be better UX to reverse the option label, so that it says 'Send abandoned cart emails' (and have it enabled by default). Because I believe that I read somewhere that enabling an option to disable a feature can be confusing.

@hswong3i
I understand that people could need this and ✨ File target: add support for local file path as source Needs work together, but for keeping the issues focussed, it is better to keep them separated. Instead, maybe it is better to open a separate branch called '2928904-2968671-media-and-local-file-do-not-merge' for that.

For people would like to see this issue to be resolved, we need to have automated tests - and there appears to be an issue with updating existing configuration. See Remaining tasks for details.

Thanks for fixing! I enabled PHP 8.4 testing and I see that the Role Delegation module has a similar issue. But there also exists an issue for that already: 📌 Nullable types must be explicit Active .

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

Great, tests are passing again. Merged the code.

megachriz → created an issue.

Can it be that there exists a content entity type here that violates \Drupal\Core\Entity\EntityInterface contract? Allowed return types for \Drupal\Core\Entity\EntityInterface::label() are string, \Drupal\Core\StringTranslation\TranslatableMarkup or null.

With the entity types I tested, all labels are instances of \Drupal\Core\StringTranslation\TranslatableMarkup.

It is the case that \Drupal\tamper\Plugin\Tamper\EntityFinder::getEntityTypes() returns a multidimensional array, but this is intentional. The list of content entity types are grouped by provider:
$entity_types[$entity_type->getProvider()][$machine_name] = $entity_type->getLabel();

This makes it easier to find the content entity type that you want to use:

@jens peter

I cannot understand why this is not sorted a long time ago

Well, it is because I'm unable to address every issue. Feeds has more than 800 open issues and I also am maintainer of a few other projects on drupal.org. While I would like to get everything fixed, I can only do so much. 🤷‍♂️

but guess the developers are from the USA

Nope, I'm from the Netherlands. 🙂 I import date values too, but apparently I'm not affected by this issue. I think that it is PHP who assumes American date formats by default.

If you like to get this issue addressed, you can help in the following way:

• By providing an automated test that demonstrates the issue.
• After that, by providing a fix that make that test pass.
• By joining the weekly Feeds meeting on Slack and help with reviewing or testing other issues that are being discussed during that meeting. The meeting is every Thursday on 19:00 UTC from November till March and 18:00 UTC from April till October.

@GGH
I see that PHPCS fixes are already being picked up in other issues, for example in 📌 Fix remaining phpcs, cspell issues Needs review and 📌 \Drupal calls should be avoided in classes, use dependency injection instead Needs review . Maybe not worth of your time to do that here again. Also, you are adding commits to a branch that can get wiped out by the update bot, see issue summary:

Warning: The 'project-update-bot-only' branch will always be overwritten. Do not work in that branch!

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

Coding standard fixes have been done. Setting to "Needs review" for now, until it is clear what changes exactly are requested in #64.

All Tamper from the D7 version of Feeds Tamper plugins are ported!

I've opened a follow-up for adding an additional configuration option (called "Return field") for the Entity Finder plugin: ✨ Entity Finder: add return field option Active

The idea comes from @hepabolu who added this option to a plugin similar to Entity Finder in https://github.com/hepabolu/feeds_tamper_lookup_entity, so it would be great if we could port that option to the Entity Finder plugin.

megachriz → created an issue. See original summary → .

I merged the code! Thanks all who contributed to this issue.

I've rerolled the patch from #62 into a MR and I made the following changes:

• Updated trait with changes from SimpleGMapFormatter.
• Use attribute instead of annotation for SimpleGMapAddressFormatter.
• Removed unused .orig file (simple-gmap-output.html.twig.orig).
• Updated tests.

Probably needs coding standard fixes (phpcs, phpstan, etc.).

@smustgrave
What's the duplicate code that you refer to? Most code of SimpleGMapFormatter is moved to a trait, so SimpleGMapAddressFormatter can make use of it. This is done to reduce code duplication. But perhaps I missed code duplication somewhere else?

Yes, with Feeds it is possible to update content that was not originally created by Feeds. In order to update content, you need to mark at least one mapping target on the mapping page as unique. Furthermore, the CSV column names configured on the mapping page needs to match exactly with the column names used in the CSV file.

How does your source look like? How did you configure your feed type? Is there a mapping target that you configured to be unique? If so, which one?

Needs a reroll (and MR) now that 🐛 Importing new files fails when "Autocreate entity" option is turned on. Active is in.

Also, I suggested a change, see remaining tasks from the issue summary.

Thanks ad0z for your contribution! I made a few more changes and I merged the code.

#3316065: Images imported from external feed often map to the incorrect node entity when several feeds are imported during the same cron run → looks like a duplicate of this one. For now I'm just linking it as a related issue.

This needs a test to demonstrate what is exactly going wrong. After that, we can work out what the correct way is to fix it.

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