🇳🇱Netherlands @megachriz

Thanks, I hope I can review this one soon.

@bobknocker
That's a pity that you run into this, but I don't know why that happens. If you are on Drupal Slack, maybe could ask in the #composer channel?

Tagging with "multilanguage"

What kind of syntax is this? Anyway, I don't know if there already is a module that can parse this format.

Since the format looks similar to XML, if you want to write a parser yourself, I recommend to create a FeedsParser plugin with a class that extends \Drupal\feeds_ex\Feeds\Parser\ParserBase. That class is part of Feeds Extensible Parsers → , so that one would be your module's dependency.

Marking this as "Fixed" because I gave an answer. Feel free to reopen if you need more information or help.

I've opened ✨ Display CSV headers in textarea when creating a new feed Active , so this feature can be tested in combination with the Feeds Textarea Fetcher → module.

I think that I completed all of the listed remaining tasks, but to be sure it would be good to check that.

megachriz → created an issue.

In September, @irinaz said in Slack that she wanted to help on the logo:
https://drupal.slack.com/archives/C34CECZAL/p1725818501416959?thread_ts=...

So I'm assigning the issue to her for now.

We'd like to go for something like the following logo:

The edit file as SVG is in the zip that was posted in #40:
https://www.drupal.org/files/issues/2024-08-22/Feeds-edited.svg_.zip →

But here is the Affinity Designer document as well:
https://www.drupal.org/files/issues/2024-11-14/Feeds-edited.afdesign.zip →

Remaining task: For the logo it should be checked if the documents has the right proportions and perhaps we should check if the strokes are thick enough. So some checks and maybe little tweaks.

smustgrave → credited megachriz → .

I created ✨ Add a setting for limiting the amount of times a lock may be extended Active
If someone wants to take a first step implementing a bit of it, that would be appreciated. 🙂

megachriz → created an issue.

I think it should default to the smallest effect on users. Thus the best would probably to use a big time frame like 12 hours. There will be few users that have a feed running longer. I would assume that these users rather check the settings description carefully and change the settings to their needs.

Alright, that sounds reasonable. This issue reported in the issue summary looks to be more about that Feeds never stops retrying fetching when it encounters a 404. While that is a possible cause for a feed getting stuck and as a result locks keep getting extended, maybe we would need to handle implementing lock settings in a separate issue?

You don't have patches applied to Feeds, by any chance?

Thanks for reporting. I think that the error is related to incompleted imports at the time of the update.

Only weird thing is that it says "undefined method". I would have expected the error to be something like "cannot call addMessage() on null" or something similar.

@thirstysix
Can you check if you see something suspicious in the error_log on your server?

✨ KeywordFilter: more config pre-processing from form validation to tamper execution Active is done! So here is a MR that will make KeywordFilter to throw a SkipTamperItemException instead of emptying a value if the data does not contain one of the configured keywords.

Hopefully, the MR passes tests.

Thanks for following up so quickly! I merged the changes.

Thanks for giving some background as well. At first I thought about if it would be useful to add token support to Tamper plugins, but since the Tamper module would be missing context (it doesn't get entities passed for example), maybe token replacement is better handled externally. I think updating/altering the config used at runtime is good enough, as long as you don't store that updated config. But I assume that you have handled that correctly.

I see that in 🐛 Properly handle arrays in tamper plugin config Active there is a special case for the "find_replace_regex" Tamper plugin. I wonder if it would be a good idea to use a different data type in the config schema for tamper.find_replace_regex.find, but on https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... → I didn't see a logical subtype of string to use. We could define our own subtype and call it "regex" for example. This is unrelated to this issue, but a thing that crossed my mind.

Here is my proposal for the field descriptions:

Retroactive update

Move and rename previously uploaded files. After saving the field settings, the paths of all previously uploaded files will be updated immediately. This will only occur once. So after the operation is done, this option will be disabled again.
Warning: This feature should only be used on developmental servers or with extreme caution.

Active updating

Actively move and rename previously uploaded files as required. If necessary, the paths of previously uploaded files are updated each time the entity they belong to gets saved.
Warning: This feature should only be used on developmental servers or with extreme caution.

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

Yes, the build form already converts an array of words to a string that is usable for the textarea. I put this back to "Needs review", so you can give it another test. Thanks in advance. 🙂

Perhaps good to know: in 🐛 Make Keyword Filter to skip an item instead of emptying the value Active I plan to make another change to the Keyword Filter, namely that it will throw a SkipTamperItemException instead of emptying a value. This is consistent with how the Required plugin works, which also can throw that type of exception, so I expect it would not cause new issues.
But I plan to make that change after this issue is done, because it would else cause merge conflicts.

My reasoning for no longer use the "words" setting:

• I think that it is better to save a list of words only once, instead of having it in two formats.
• The tamper() method did not use the "words" setting (now it could use it, but only for backwards compatibility and only if the new setting "words_list" is empty).
• The tamper() method wants to have an array of words.

Converting the list back to words isn't simple either.

buildConfigurationForm() easily converts an array of words to something usable for a form:

'#default_value' => implode("\n", $this->getWordList()),

So upon submitting the form, the words inputted in a textarea are converted to an array. And when loading the form, the array is then converted back to a string.

@jurgenhaas
Does this cause any issues for you? Would you still need a processConfigValues() method?

@jurgenhaas
Note that the "words" setting (where words are saved as string) is only respected for backwards compatibility. When (re)saving the configuration, the setting will become an empty string.

Example

Before (configuration in the old format, from a feed type):

words: |-
  foo
  bar
word_boundaries: true
exact: false
case_sensitive: false
invert: false
word_list:
  - /\bfoo\b/ui
  - /\bbar\b/ui
regex: true
function: matchRegex
uuid: 81082ad2-4c8a-44c8-aeff-2969eb3f9612
plugin: keyword_filter
source: qux
weight: 0
label: 'Keyword filter'

After (configuration in the new format, from a feed type):

words: ''
words_list:
  - foo
  - bar
word_boundaries: true
exact: false
case_sensitive: false
invert: false
uuid: 81082ad2-4c8a-44c8-aeff-2969eb3f9612
plugin: keyword_filter
source: qux
weight: 0
label: 'Keyword filter'

I also fixed some phpcs and cspell issues.

megachriz → created an issue.

I'm testing this myself. One thing that I think is not correct is that if you have the permission to import your own feeds, but not feeds you do not own, you can still access the CSV template of other feeds. Also, I think it makes sense that you should also be able to access if you can create or update feeds, even if you cannot import them.

What I'd like to change as well is the path for the feed specific template. Currently this is /feed/template/{feeds_feed_type}/{feeds_feed}, but I think it makes more sense if it was /feed/{feeds_feed}/template instead. This requires also additional access control and tests.

The feed type specific template can stay to be /feed/template/{feeds_feed_type}

So, work to be done!

I see: group relationships should get cleaned up upon group removal. Perhaps it is caused by a local issue then. Or maybe I accidentally patched the wrong file? Because I see that my MR is different from the patch in #3.
Good call on requesting the steps to reproduce the issue first.

@anybody
Thank you for testing and for your compliments! 😊
Now that I'm mostly done porting my projects to Drupal 11 (I decided to take a break on that for the remaining) and all Feeds stable release blockers have been resolved, I can finally pick some issues that I had put on the wait list. So I sort of randomly picked up this as it looked like it could be resolved in a relative small amount of work hours.

Tests are passing!

One thing I'd like to see added is a method for returning the raw template contents - as a string. This can then be used by Feeds Textarea Fetcher → . With that module you can put in the source data in a textarea. It would be nice that in case of the CSV parser, it would already display the CSV columns. And that is only possible if Feeds Textarea Fetcher can load the raw template contents.

For now I set this to "needs review" because it is ready to be tested.

I found this example for combining values using Xpath: https://scrapfly.io/blog/how-to-join-values-in-xpath/

So it looks like that the Xpath that you need is:
concat(bc:location/bc:street, " ", bc:location/bc:number)

Upon further looking at the stack trace, the array_flip() error did not happen in \Drupal\feeds\Entity\FeedType::__sleep(), but in \Drupal\Core\Entity\EntityStorageBase::loadMultiple() instead:

  /**
   * {@inheritdoc}
   */
  public function loadMultiple(?array $ids = NULL) {
    $entities = [];
    $preloaded_entities = [];

    // Create a new variable which is either a prepared version of the $ids
    // array for later comparison with the entity cache, or FALSE if no $ids
    // were passed. The $ids array is reduced as items are loaded from cache,
    // and we need to know if it is empty for this reason to avoid querying the
    // database when all requested entities are loaded from cache.
    $flipped_ids = $ids ? array_flip($ids) : FALSE;

The error looks like to be triggered by either an entity reference field or a file field. While theoritically this could come from a bug in Feeds, it is not clear if this is the case. Since this issue is over 3 years old now, I think I'm just going to close it. Feel free to reopen if you still encounter this issue and if you can provide the steps to reproduce the issue.

Note: I did merge a change for FeedType::__sleep(), because the double usage of array_flip() was not needed.

I plan to work on this, have drafted some locally, but first ✨ KeywordFilter: more config pre-processing from form validation to tamper execution Active needs to land.

I've merged 🐛 Keyword filter: variable executed as function, security issue? Active which should fix part of this issue.

Here I went further by removing all config processing in validateConfigurationForm(). Now that method only validates. There is still a bit of config processing left in submitConfigurationForm() and that is that the list of words as inputted on the form is converted to an array. For example: "Foo\n\Bar\nQux" is saved as ['Foo', 'Bar', 'Qux'].

Summary of changes:

• Deprecated 'words' setting;
• Removed 'word_list' setting;
• Added 'words_list' setting as a replacement for 'words' and 'word_list';
• Determine regex to use when applying the Tamper plugin and not when saving the config;
• Added backwards compatibility for the 'words' setting.

I drafted a change record that also includes the changes from 🐛 Keyword filter: variable executed as function, security issue? Active :
https://www.drupal.org/node/3485191 →

@jurgenhaas
Would this work for you? Or would you still need something like processConfigValues(), basically just for converting "Foo\n\Bar\nQux" to ['Foo', 'Bar', 'Qux']?

Merged the changes!

I know that multilingual support in Feeds is not perfect yet, but since I don't have clients with multilingual sites currently, I prioritize other Feeds issues as of now. Unfortunately, I don't know a good workaround for this issue. Perhaps making 'Authored by' translatable or using a separate feed type for this field? I would need to dive in deeper into this topic in order to think of better workarounds.

Note: the provided link results into "Preview Not Found". Was it only available for a very short amount of time?

Tagging issue with "multilanguage", so I can find this issue back as soon as I plan to focus on Feeds multilingual issues again.

This could be a Composer issue. I don't know a lot about fixing Composer related problems. One thing that helped me sometimes when I faced a Composer related issue like this, is deleting the vendor library ("laminas" in this case) and the module directory ("feeds" in this case) and then try again with either composer install or composer update drupal/feeds.

Note that on the automated test runner, Feeds installs fine with laminas/laminas-feed (2.23.0), so that's why I think it is more likely to be a Composer issue than a Feeds issue. See https://git.drupalcode.org/project/feeds/-/jobs/3183360

To be certain, the "laminas/laminas-feed" is installed the site's vendor directory? Thus in site/vendor, not in site/web/modules/contrib/feeds/vendor?

I'm not sure if I want to require Drupal 10.2 yet, so because of that it might be useful to add test coverage for this.

I do indeed get no Tamper plugin to choose from (using Feeds Tamper):

I think that TamperManager needs a change.

For example, \Drupal\filter\FilterPluginManager in Drupal 11.x has a reference to the attribute class:

use Drupal\filter\Attribute\Filter;

Maybe this would become less confusing when ✨ Add a CSV template feature to download a sample CSV file Needs work is done. Then, on the feed form, the CSV columns using in mappings would be listed.

Alright, good idea. I think that we can only test this by writing a custom Tamper plugin? Maybe it would be good to have a Tamper plugin using attributes in a test module, so that we can add test coverage for this addition?

I merged the code for the issue you had with feeds_log and since it looks like your other issues with the module are now also solved, I'm marking this issue as "fixed".

Feel free to reopen or open a new issue if you experience more issues related to the MS SQL Server Driver. 🙂

Great, all things are green.

megachriz → created an issue.

Merged the code!

An other possibility is to throw an exception in GroupRelationship::getGroup() in case the group entity could not be loaded. That would be more descriptive than the TypeError that we get now.

Something like this:

/**
 * {@inheritdoc}
 */
public function getGroup(): GroupInterface {
  $group = $this->get('gid')->entity;
  if (!$group instanceof GroupInterface) {
    throw new \RuntimeException(sprintf('Group %d could not be loaded.', $this->get('gid')));
  }
  return $group;
}

@kristiaanvandeneynde
This error occurs when the group is deleted, but the group content still exists. Perhaps it would be better to deny access to the content if the group can no longer be found?

This suggested change by the update bot is not useful. It breaks D9 testing and it adds a backwards compatibility call for something that is already a backwards compatibility call.

Because there is a time limit on cron runs, if your import is large, it can take multiple cron runs to complete an import. Feeds is given about a minute per cron run to do its thing.

I guess I already addressed this issue before, I just did not finalize it yet: 🐛 PHP notice when passing NULL to the TruncateText plugin Needs review

All tests are passing after fixing a few other issues first!

Test coverage for Commerce Recurring would be a nice to have, but we could also handle that in an other issue. I don't use Commerce Recurring, so I do not plan to write these tests myself.

Then we may need to answer the following question still: is it enough to only test for mail to be not null or can it be an empty string too?

Drupal 9 testing has been enabled again! Now we don't yet need to remove Drupal 9 support.

Drupal 9 testing works, merged the code! There are issues in other parts of the pipeline, but that is out of scope for this issue.

megachriz → created an issue.

megachriz → created an issue.

gábor hojtsy → credited megachriz → .

Is there something scheduled to be imported? If not, try to start an import using the “Import in background” link and then run cron. If the feed is currently locked, unlock it first and then try to import in background.

Note: A feed becomes locked when an import for it is started or planned, to prevent running two imports for the same feed at the same time. But if an import abruptly ended due to an error, the feed may remain in a locked state without the import getting continued. In this case, it is good to unlock it manually.

If you click the "plain diff" link, then you get a text file that you could use as a patch.

I do not have a MS SQL Server driver ready, but based on the error message, I made a change that perhaps works. The error says "Cannot update identity column 'lid'", and the code is passing a value for 'lid' when updating a log entry. So maybe the issue gets fixed when the code no longer passes a value for 'lid'.

Can you try the code from 3482189-cron-sql-server-driver to see if that helps?

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

I see that the url you provided, for example returns the following item:

"tick": "GEME",
"max": "2100000000000000",
"lim": "10000000000",
"pre": "0",
"to": "kaspa:qpw62u93wvng9t3kjzmp3cyr37zwkwppjf7l0s7kc7yhp08xmqu3kqguyhgap",
"dec": "8",
"minted": "151520000000000",
"opScoreAdd": "932033290104",
"opScoreMod": "932041480279",
"state": "deployed",
"hashRev": "b2a756ac793d78d689be807274786f91a2ef515e01bac2acd68224057026466e",
"mtsAdd": "1729508652381"

Each property of the above could be mapped to a field defined on the entity type that you are importing data for.

For example, the JSON souce "tick" could be mapped to node title.

Does this answer your question? If not, feel free to reopen this issue. Maybe provide a screenshot in that case, because I am not sure what you mean with "I do not see any Feed Item options in the mapping target.".

Improvements that could possibly be made:

• A setting for making the link valid longer than 24 hours.
• A setting to not expire the link immediately when clicked (it could be that this requires a change in the Commerce Checkout Link module).

Merged!

See also #3481787-3: Large feeds do not import entirely →

Okay, then I'm closing this one!

@maintainer
It would be best if HttpFetcherPost no longer needs to override the fetch() method. But perhaps that requires code from HttpFetcher::fetch() to be split up further. Let me know if you need any changes to that class to make the implementation for this module easier.

This particular feed does use feeds_fetcher_post module

Ah, then that must be it. I see that feeds_fetcher_post does still uses the previous directory style, which thus can cause issues on Pantheon.

Here that code can be seen:
https://git.drupalcode.org/project/feeds_fetcher_post/-/blob/2.x/src/Fee...

If feeds_fetcher_post could override less of the HttpFetcher than it does now, then this issue can also be fixed when that fetcher is used.

I thought I had fixed this issue, see 📌 Missing temporary files in load balanced environments Fixed . /tmp/feeds_http_fetcherXxx is also the old location were the temporary file got stored. The file to import should be stored now at private://feeds/in_progress/[feed id]. If it is still /tmp/feeds_http_fetcherXxx then the most logical cause of that is that you updated from an older version of Feeds while there was still an import task on the queue. If so, unlock the feed in question and retry to start an import. If you cannot unlock the feed because it is not locked, make sure that all feeds tasks in the queue table are removed first. Then you should be able to start fresh with imports and temporary files will be stored at a location that hopefully doesn't cause issues on Pantheon.

Can it be that you updated from 8.x-3.0-beta3 recently? Because the issues was fixed in 8.x-3.0-beta4, though only for new imports, not for unfinished ones that started before the update to 8.x-3.0-beta4 or later.

megachriz → created an issue.

I see it is failing tests. Perhaps it would be a good idea to create a functional test base class in a separate issue first.

I've worked on this for the 2.1.x version of Commerce Abandoned Carts.

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

I see that you removed properties from AbandonedCartMail and AbandonedCarts. Was that intentional?

Ah, I see now that the properties are not removed, but are defined in the constructor instead:

  public function __construct(
    protected Connection $connection,
    ConfigFactoryInterface $config_factory,
    QueueFactory $queue_factory,
    protected ModuleHandlerInterface $moduleHandler,
    protected TimeInterface $time,
    protected LoggerInterface $logger
  )

I'm not used to this pattern yet, but I agree it is the modern way forward.

It would be useful to add test coverage for this issue. Also, code now needs to be in a merge request instead of a patch because patches are no longer evaluated by the drupal.org testbot.

I fixed the branch tests in 📌 Fix tests for Drupal 11 / Commerce 3.x Active and then I merged 2.1.x into this issue fork. Let's see what that does for the tests.

2.0.x passing too. Merged that one as well.

Okay, for 2.1.x the tests no pass again! Merged that. Now let's see if the same type of fix can be used for 2.0.x as well.

megachriz → created an issue.

I see that the recent change is noted on the release on the Commerce Core project page:

The cart module no longer depends on commerce_product.

So I think that in the tests we need add a to dependency to 'commerce_product'. I shall create a new issue for that.

The test failures only look like to happen for Drupal 11 + Commerce 3.x. Maybe something changed in Commerce 3.x recently that caused the tests to fail?

Is it enough to only test for mail to be not null or can it be an empty string too?

Good suggestions.

About the first: history_limit must be lower than commerce_cart[cart_expiration][number], I think that it would be better to only warn the user that carts don't exist if they set history_limit to a higher value and not enforce that it must be lower than commerce_cart[cart_expiration][number].

Second: good call to clean up records for no longer existing carts.

@abhishek_gupta1
Thanks for your contribution, but I think you picked the wrong file to adjust. The file to adjust is TruncateText.

Also, I think that a test case needs to be added to \Drupal\Tests\tamper\Unit\Plugin\Tamper\TruncateTextTest to check if an empty string gets returned when NULL is passed to the plugin.

Also note that the drupal.org testbot no longer evaluates patches. So before a fix can be accepted, it first needs to be in a merge request.

Thanks for your work so far! I see that you removed properties from AbandonedCartMail and AbandonedCarts. Was that intentional?

I think that something like solution #1 makes more sense. However, not with using empty() as that will make 0 into an empty string too. I think we can even apply solution #2 as well - in the case that an object is passed. Then the Tamper would attempt to cast that object to a string.

This needs to be fixed in the Tamper project though, so moving this issue to that project.

@chrisgross
Agreed that this needs some work. There is now only a message in the logs about the lock getting extended. Perhaps there indeed needs to be a maximum lock time in the form of a config option.

Do note that the lock only gets extended if Feeds thinks that the import is still active. So perhaps that detection mechanism could be finetuned too. The lock gets extended when:

• There are still tasks for the feed in question on the queue.
• There was progress being made in the last hour.

So I think that the underlying issue is actually that there are tasks on the queue that are getting stuck. There could be different reasons for this as said in 🐛 Processing of failed queues never finish, feeds import gets stuck Active . Finetuning the detection mechanism would then thus be: is there a way for Feeds to know if the tasks on the queue got stuck?

But a config option for a maximum lock time would nevertheless be useful. I only don't know what it should default to. If it is set to 'Never' by default, then you may only think about setting the option when you stumble upon this issue. If it is set to '12 hours', then that could be an issue for people running very large and long imports and they then detect after 12 hours that their import suddenly aborted. I think that Feeds cannot know at the start whether it gets fed a source that is going take a long time. Because not only this depends on the size of the source, but also on how often cron runs.

I see that this happens when the configured upload directory on the feed type is not writable.

Go to your feed type, and on "Fetcher settings", check which upload directory you have configured there. Then check if the file permissions on your system are set correctly for that directory. Or the parent directory if the directory does not exist yet. If it is set to "private://feeds" (the default), then your site may have an issue with writing files to the private file system. If it is set to "public://feeds" (the default if there is no private file system), then there is an issue with the public file system instead. You can see on /admin/config/media/file-system which directories are used for the public and private file system.

As for the log message, this is caused by a bug in Drupal Core: #1903010: Notice: Undefined index: #field_name in file_managed_file_save_upload() → .

Do you have any additional file related modules installed? It has been reported before that one messed things up for Feeds. I don't remember which module it exactly was, but it could be that it was "File Entity".

Thanks for your contribution. I merged the code!

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

I'm not sure if I understand your question. I read from it that for the source RSS:

• GUID is sometimes provided, but not always;
• Title is not unique.

You combine title+GUID, but because GUID is not always available and title is not unique, this is not guaranteed to provide a unique value.
You then combine title+"the feed item's timestamp" which is probably always unique, so you cannot update items that way.
Perhaps you could use title+"created date" as unique or is 'created date' not available in the feed?

I wonder what Aggregator has to do with this. Since you wrote it with a capital letter, do you mean the Aggregator module → ? By my knowlegde Aggregator and Feeds Tamper do not work together.

Can you rephrase your question?

All tests pass. Merged the changes!

I turned this into a merge request, so that the changes are evaluated by the testbot. The testbot on drupal.org has stopped testing patches, so the MR workflow is now required if you want that your changes are evaluated by the testbot.

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

Would it be better to reverse the option? Instead of it saying "Do not reset hash", let it say "Reset hash". I'm not sure, but I think it's better UX that enabled options imply to "do" something instead of "do not" something. Opinions?

Also, since there is a patch, the status "Needs review" would apply here. Though it would be better to create a MR from it, because the testbot on drupal.org no longer evaluates patches.

It would be nice indeed to check how Migrate approaches this problem.

Note: in 🐛 Keyword filter: variable executed as function, security issue? Active I proposed to move some parts indeed to the tamper() method. At least, it proposes to remove the function from the configuration and determine that when tampering instead. Perhaps the processing of words should also take place in tamper().

There are more Tamper plugins on which data is processed in validation/submission, for example:

• aggregate
• convert_boolean
• find_replace_multiline
• find_replace_regex

I don't think that we should store the config exactly as inputted on the form for the keyword filter plugin. We could however add public methods that can process the information outside of the form context. Would that work for you?

Sorry for the late reply, by the way, but I had put most Tamper issues on hold for a long time in order to focus fully on Feeds.