🇳🇱Netherlands @megachriz

megachriz → created an issue.

I exported the definitions from before this change and after this change and then compared the two files to see if I spotted any differences that would be an error and did not see errors. There were slight differences in the plugin's descriptions. For some plugins, single quotes in the description were replaced with double quotes. But that's no error.

I used the following code for exporting the definitions:

drupal_flush_all_caches();

$keys = [
  'id',
  'label',
  'description',
  'category',
  'handle_multiples',
  'itemUsage',
];

$rows = [];
foreach (\Drupal::service('plugin.manager.tamper')->getDefinitions() as $definition) {
  $row = [];
  foreach ($keys as $key) {
    if (!array_key_exists($key, $definition)) {
      $row[$key] = 'undefined';
    }
    elseif (is_object($definition[$key])) {
      $row[$key] = (string) $definition[$key] .  ' (' . get_class($definition[$key]) . ')';
    }
    else {
      ob_start();
      var_dump($definition[$key]);
      $row[$key] = ob_get_clean();
      $row[$key] = trim($row[$key]);
    }
  }
  $rows[$definition['id']] = $row;
}
ksort($rows);

file_put_contents('/tmp/defs2.txt', print_r($rows, TRUE));

This is good, so I've scheduled to merge the changes.

And I see lots of test failures with Drupal Core 11.3.x-dev, while all tests pass with Drupal Core 11.2.8. 🤔
I hope my client will give me some time to help with this.

Importing multilingual content with Feeds is indeed not perfect yet. There are more language related Feeds issues. I tag these with "multilanguage", so I can find them back as soon as I plan to focus on Feeds multilingual issues again.

Closing this one as a duplicate of 💬 Invalid enclosure Active

Let me see if I understand things correctly:

1. An external process places a XML file and a jpg file into public://feeds.
2. The image file is always called "art-00.jpg".
3. You use Filefield Paths module to rename the file, but that is configured in a way that previous files could get overwritten.
4. Because of that you, you've disabled the Filefield Paths configuration and are now trying to rename the file with Feeds instead.

I think that the issue is this:

1. Feeds only supports renaming a file during the process stage and only by numbering them. At the process stage, Feeds writes the data that it received.
2. Tampers (including the Rewrite Tamper) are applied during the parse stage. If you apply a Tamper on a source that contains a reference to a file, you are instructing Feeds where to look for the file to download. This does not rename the file itself.
   You are saying to Feeds something like: instead of looking for a file at "public://feeds/art-00.jpg", look for a file at "public://feeds/Hania Rani - Portico Quartet & Hania Rani EP.jpg" instead. And Feeds reports back "Hm, I cannot find that file.". The file that is there is still called "art-00.jpg".

I don't know if there is a way to configure Filefield Paths that it will not overwrite existing files. I think your best option is to include something in the file path which would make the image name unique. That could be the node ID for example or something else that uniquely defines the album image.

But it sounds like that files getting a new ID on every import is an issue that you encounter. I don't see a direct solution for that problem. I know there exists an unresolved issue when using Feeds and Filefield Paths together: namely that on every import a new file gets created. It is an issue in the 8.x-3.x version of Feeds too.

I hope this helps you further.

foxy-vikvik
Yes, you are right that an example for QueryPath should be added to the documentation.
Do you want to add one on https://www.drupal.org/docs/contributed-modules/feeds-extensible-parsers... → ? I could add one too, but it might take a while before I'll get to it.

A working solution for this feature currently exists in my sandbox Feeds DEV → . I've been using that for years with success. Only thing what needs to be done is moving the code to an official project (probably my other sandbox Feeds Base64 images → [which would need to become an official project]) and fix any code style issues.

When importing files, you need two source fields:

• A source field with the base64 string
• A source field with the file name

There is one caveat with the solution provided there: all file names need to be unique - even if the files are saved to different folders. That's because all files are saved to a temporary folder first - and if file names are not unique, it would overwrite previously assembled files.

megachriz → created an issue.

I've updated the conversion:

• category is made translatable.
• new property itemUsage is now converted too.
• plugins that were added this year ('absolute_url', 'entity_finder' and 'twig') have now also conversions.

Do the changes in the MR help to resolve the issue?

Thanks for merging!

Just so you know, on the MR pipeline there is now an option to run tests without the fix. It also runs only the tests in files that were modified.

Is this still an issue?

If so, I need more information on how you configured the feed type in question and how you configured any tampers. I think that it is most likely caused by a configuration error.

I've provided a test and a possible fix in the MR.

megachriz → created an issue.

Hm, for this particular plugin, the defaults should be NULL. I think that's why defaultConfiguration() is not defined. The method getSetting() would return NULL if the setting key is not defined.

Is a checkbox field not allowed to have NULL as default? Then it should probably become FALSE if $this->getSetting(static::SETTING_FALLBACK) returns NULL.

Current code for the checkbox field:

$form[static::SETTING_FALLBACK] = [
  '#type' => 'checkbox',
  '#title' => t('Fallback to strtotime() if the date could not be parsed with the provided date format.'),
  '#default_value' => $this->getSetting(static::SETTING_FALLBACK),
  '#states' => [
    'visible' => [
      'input[name="plugin_configuration[date_format]"]' => ['filled' => TRUE],
    ],
  ],
];

(note: I see it is calling t() instead of $this->t().)

But I'm willing to define defaultConfiguration() just with both setting keys set to NULL. Would that help?

@foxy-vikvik
Based on the error message I see that you are using one of the QueryPath parsers. I see that an example for these parsers are not yet documented on https://www.drupal.org/docs/contributed-modules/feeds-extensible-parsers... → .

I don't use the QueryPath parsers myself, but based on the information in the tests it looks like to work in a similar way as how you would select elements with CSS on a HTML page.

For example in the following HTML the div could be selected by .post, by div, by html body div or in a few other ways.

<html>
  <body>
    <div class="post">

So instead of using /rss/channel/item you could use rss channel item or just simply item.

You can also use the XML Xpath parser instead. Then the context value that you provided in the screenshot should be valid.

It seems this module is not actively maintained.

Yes, I saw I hadn't enabled notifications for this project yet. But it is true that I give this project very minimal attention. Most of my attention goes to Feeds. For Commerce Feeds I mostly only come back for when it's time to make it up to date for the next version of Drupal Core.

This bug does look worthwhile to fix. Does this occur when there are no currencies configured in Commerce?

I think the fix and the tests are good enough.

I cannot remove febbraro as he is the owner of the project node.

I went ahead and removed those that haven't been active in the last 10 years in this project. @twistor was last active I think in 2016 and @franz did contribute to Feeds 1.5 years ago. So I haven't remove them.

I went ahead and removed those that haven't been active in the last 10 years in this project. @twistor was last active I think in 2016 and @franz did contribute to Feeds 1.5 years ago. So I haven't remove them.

Oh, I see I had created an issue for this as well: 📌 Cleanup list of Feeds maintainers Active . I thought last commit 10 years ago was a fair reason to remove them from the list.

I assume that the cause was that the available update information wasn't updated yet on your site. 8.x-3.2 was only released yesterday. And not long before that, 8.x-3.0 was the latest release. (The 8.x-3.1 release was the latest release for only one minute.)

Thanks for closing this issue. 🙂

I mean the "Check manually" link on /admin/reports/updates:

drush cr does not refresh the list of available updates I think.

By the way, when I install Feeds (not update it) on an existing Drupal site, I get this:

Ah, that's weird. Can you try to refresh the update data? (There is a button somewhere in the admin interface for that.)

Merged.

In the issue fork no GitLab issues. Go figure. 🤨

Well, let's fix and merge 📌 Fix PHPStan issues Active and then we'll see again.

megachriz → created an issue.

megachriz → created an issue.

@nicolasgraph
I have created a 8.x-3.1 → release that restores Drupal 10.1 compatibility. This is basically what is in the 8.x-3.0 release + the changes from the 3553772-10.1-support-2 branch. So this includes the changes that you posted in #8.

I also released 8.x-3.2 → that now requires at least Drupal 10.2.

I've scheduled the merge.

Alright!

Interesting request. I do wonder if you could also do the following instead: add the "Default value" plugin where you enter the values separated by comma in a long string and then follow it up with the "Explode" Tamper plugin, where you split the values by comma. I think that would also work?

PHP 8.4 testing is now enabled on the project.

The tests do pass at least on PHP 8.4 + Drupal 11.2.5. Maybe there was a hickup in the installation process - one that doesn't happen every time. If PHP 8.4 is passing code style checks as well, I merge the MR and mark this as "Works as designed".

Alright, thanks for reporting that back.

I do see that Feeds isn't automatically tested on PHP 8.4 yet. So we could check if Feeds tests pass on it.

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

For your interest, in 3135359-temp I made an attempt to run tests for both CommonMark v1 and v2, so that we could get test coverage for ✨ Add support for Commonmark v2 Active . Since the tests weren't merged before, I temporarily experimented with it in this issue instead.

So ideally, the commits from that temp branch that are worth it, should move to ✨ Add support for Commonmark v2 Active .

@joelpittet
Ah, thanks for merging! That work project that I talked with you about on Slack last week has been demanding all my work time in the past 6+ months, so I wasn't able to continue with Markdown during office hours.

@damienmckenna
Thanks for reporting that back. If it happens incidentally, but it does continue to happen, maybe the Feeds Log could perhaps give an indication what might go wrong when it happens again? "Feeds Log" is a module included with the Feeds project. For every import, it logs which items were created, updated, cleaned (deleted or unpublished) or ignored. By default, the logs are kept for one week because it can potentially store a lot of data (when you have very large imports).

@damienmckenna
Could your issue perhaps be related to 🐛 Wrong clean action applied when running imports for multiple feeds in one request Active ?

This issue is not fixable at the moment. The library does indeed have marked the interface as deprecated, but it also doesn't work without it yet.
See https://github.com/laminas/laminas-feed/issues/85

@andriy khomych
Thanks for confirming! Also thanks for your contribution and your patience to get this issue resolved. I scheduled the MR for merging.

I think that this is good to go! Changes that I made:

• Expanded description of CronTest::testImportPeriodPerFeed() a bit more and also added a few more comments in the test.
• When creating or editing a feed, I noticed the UI showed an option called "- None -". I think this was confusing because that option did essentially the same as "Use the feed type default", so I removed that option in FeedForm. I also set the default value of the field to "Use the feed type default" in case no value was set yet.

Is it good to go for you too?

megachriz → created an issue.

Thanks for the update! I hope to review your work this Thursday or else Saturday.

Great! Tests are passing again. Scheduled for merge.

megachriz → created an issue.

I've done a new review!

I've checked the failing test and added $this->printMessages(); right before the failing line. printMessages() is a method defined in FeedsCommonTrait and can reveal why an import failed by printing the error messages that were made during the import.

The error message said:

The content test 1 failed to validate with the following errors:

• field_file.0: You do not have access to the referenced entity (file: 2).
• field_image.0: You do not have access to the referenced entity (file: 1).

I found out that the failing tests are related to these pieces of code in Feed::basePeriodicImportFieldDefinition().

$cron_required = [
  '#type' => 'link',
  '#url' => Url::fromUri('https://www.drupal.org/docs/user_guide/en/security-cron.html'),
  '#title' => t('Requires cron to be configured.'),
  '#attributes' => [
    'target' => '_new',
  ],
];

->setDescription(t('Choose how often a feed should be imported.') . ' ' . \Drupal::service('renderer')->renderRoot($cron_required))

Commenting out the setDescription() call made the test pass on Drupal 10. I think rendering this element in a Kernel test doesn't work or at least it is causing issues.

I propose to move the description of the field to FeedForm::form(). And in the form code, the call to renderRoot() will not be needed.

Or since the messages are more than just a label, it should become this instead?

default_message:
  type: text
  label: 'Premium message'
  translatable: true
messages:
  type: sequence
  label: 'Message per content type'
  sequence:
    type: text
    translatable: true

I think that translatability of the message is currently not implemented.

It looks like that all is needed to add that support is by marking specific configuration properties as translatable in the config schema. See the documentation: https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... →

According to the docs, a config property should get translatable: true. However, the docs also say that properties that are of type 'string' should become of type 'label' to make them translatable.

So this might do the trick in nopremium.schema.yml:

default_message:
  type: label
  label: 'Premium message'
messages:
  type: sequence
  label: 'Message per content type'
  sequence:
    type: label

Can you try if this works?

This has been fixed in ✨ Improve handling of empty data Active .

I assume that this issue was in the Tamper module in ✨ Improve handling of empty data Active . In the Tamper module handling of empty data has been improved. In case of the Find Replace tamper, applying gets skipped if the input value is NULL or an empty string.

Feel free to reopen if this is still an issue.

I think that this issue was fixed in the Tamper module in 📌 Plugin Math: just integer values are accepted Fixed . If not, feel free to reopen.

If this is still an issue, feel free to reopen.

Thanks for your work!

I see that the feature is implemented a bit differently than in the D7 version. In the D7 version an additional switch is made when the "authorize" option is enabled. Because when there is being mapped to author (for example 'uid' for nodes), the author could be different from the value you configure in "Owner".
The switch in the D7 version happens upon validating. See FeedsNodeProcessor::entityValidate():

// When the import should be authorized, make an extra account switch.
if ($source && $this->config['authorize'] && !empty($entity->uid)) {
  $author = user_load($entity->uid);
  $switcher = $source->accountSwitcher->switchTo($author);
}

I do like the way you implemented the solution, by grabbing the account ID from the feed type settings. But I wonder if it would be a better idea to go for the solution that was chosen in the D7 version.

Automated tests

It would be cool to have automated tests for this feature. In the D7 version a similar test can be found in the FeedsAccountSwitcherTest class and is called testAuthorizedImport().
Are you comfortable with writing tests? I could port the test too.

Thanks for your work! I've added some comments on the MR in the GitLab interface.

Besides that, here is more from my review:

• Feed form
  1. I think that the term "importer" is not used in Feeds 8.x-3.x. So instead of "Use importer setting", would "Use the feed type default: @default" be better? It would be great if it is displayed what the default import period is. Since this cannot be directly done in the static method basePeriodicImportFieldDefinition() because of lacking context, I think that the label should be "Use the feed type default" there. And an override of the label (that also tells what the default import period is) could then be best implemented in \Drupal\feeds\FeedForm::form().
• Feed View
  1. When I have an import period on the feed, if I then disable "Allows import period per feed" on the feed type, the import period is still displayed on the page. I think that as soon as you disable "Allows import period per feed", the field value should no longer be displayed.
• Import testing
  I tested the import setting on the feed. When I set it on the feed to "As often as possible", while the feed type has "Every 1 hour", an import does indeed happen on every cron run. So this works!
  I also tested setting the import setting on the feed first to "As often as possible" and then I disabled "Allows import period per feed". The first time I ran cron, an import happened (which is okay I think) and when running cron again a few times no more imports happened. So this works good as well.
• Additional tests
  I think it can be useful to have tests also for the following:
  1. A functional test that ensures that the "Import period" field is displayed on the feed form when "Allows import period per feed" is enabled on the feed type.
  2. A functional test that ensures that the "Import period" field is not displayed on the feed form when "Allows import period per feed" is disabled on the feed type.

  I have suggested a few more tests on the MR.

Yes, only checking if the source has changed is how the hash check is currently designed indeed. Parts of the entity that was created or updated may be changed after import (including fields not touched by Feeds).

But your feature request does sound useful. Just keep in mind if you would make a hash of the whole entity, unnecessary updates could occur if any other field values are changed that were not manipulated by Feeds. There are quite some modules that could make updates on entities. I think "Content Moderation" does that for example. Because of that I'm not sure yet if we should add this feature directly to Feeds or if it would fit better in a contrib module that extends Feeds.

@byronveale
Yes, I had a great DrupalCon! I learned some stuff about DrupalAI and I helped with mentoring on the contribution day. I'm planning to experiment with AI and I hope I connect it with Feeds. I think it would be cool if AI could figure out how to configure mapping based on a (CSV) file that you supply.
Yes, I hope I can look at this soon. Including this, I now already have a least five things to look at (shortly) after DrupalCon. 😅

The MR is scheduled to be merged.

Thanks for spotting! I see that in 📌 Automated Drupal 11 compatibility fixes for feeds Needs review the testbot on Drupal 10.2.2 and not on Drupal 10.1.x, so that's why the bug was not catched. If I read the change record correctly, the change is required for Drupal 11, so upping the core requirement is the right way to fix it.

Unfortunately, it looks like we cannot fix the issue for people on Drupal 10.1, unless we create a new release with the patch from #8 and Drupal 11 compatibility removed.

@igorgoncalves
No, this one was lead by Rachel and Chris.

@rachel_norfolk
Yes, you certainly were there. You and Chris were the leaders of the session. Your most important message during the session was that the goal was that participants should progress an issue, not necesseralily fix it.

I indeed attended the issue triage session. I had selected two or three issues for the mentored contribution and marked another issue as no longer "novice".

I attended the first mentor orientation session (but not the second one on Wednesday).

I marked this issue as "fixed" because I tried to answer the question. Feel free to reopen and turn it into a feature request. Though it is possible a feature request like this was made before.

No, currently there is no option for that. The only way right now to not update a field is to not include it in mapping.

A possible workaround perhaps is to have two feed types:

• One that includes mapping to "title" and that is configured to only create items, not updating them.
• One that doesn't have mapping to "title" and that is configured to only update items, not creating them.

Does this help?

Thanks for your offer!

This project mostly needs help with addressing issues from the issue queue.

Managing releases and updating the project for new core versions of Drupal so far have been going fine. In fact, sometimes releases need to be coordinated with Feeds → releases. Whenever there is an API addition in Feeds that Feeds Extensible Parsers could use (or even wants to use), the implementation already happens in Feeds Extensible Parsers while that API addition only exists in a dev version of Feeds. This is done as a proof of concept. So sometimes a new Feeds release needs to happen before creating a new release of Feeds Extensible Parsers, because the dev version of Feeds Extensible Parsers could in theory be no longer compatible with the last Feeds release. Note that in the past few years this situation has become rare, it has happened more in the early D8 days of the project. Anyway, releases should happen in consultation.

With you already managing 50+ modules, do you have time for helping resolving issues from the issue queue?

I see in that the two failed update tests are using the 'feeds-8.x-3.0-alpha6-feeds_installed.php' fixture, in which the tables 'feeds_feed' and 'feeds_subscription' are not installed. These table are installed in the fixture 'feeds-8.x-3.0-beta1-feeds_installed.php'. So I've added the tables to 'feeds-8.x-3.0-alpha6-feeds_installed.php' as well now. Let's see if that fixes the tests.

I hope to review/test your code soon! I would like to have that done by Sunday.

I've made two updates to the code:

• Test coverage is added for confirming that the media is available on the media edit form (resolves #112). It is still possible that the media isn't displayed on the edit form, but that could be caused by a misconfiguration on the form display settings for the media type.
• A post update is added to update existing media target configuration + test coverage (resolves #108).

Still to be done:

• Add support for media reference fields that allow more than one media type.
• Add test coverage for referencing existing media.
• Add a test for importing multilingual media.
• Add support for importing remote videos.

I'm considering to handle these in follow-up issues. If so, it would be good to document on the media target that:

• There is an issue when trying to import data for media reference fields that allow more than one media type.
• Importing remote video's is not supported yet.

I set the status to "Needs review" because I'd like to hear your feedback. :)

The maintainer and I are already working on this issue.

The next step is to find out how to reproduce the issue. Probably by defining an email field in a custom module with a #ajax attribute. Requires some familiarity with the Ajax subsystem.

The next step is to add the deprecation notice: find the place where the method assertWaitOnAjaxRequest() is defined and add the deprecation notice there as explained in the proposed resolution.

I also updated the issue summary with the list of steps to resolve the issue.

I discussed this issue with the mentoring team. Tests writing is fine for novice tagged issues, but the problem here is that the issue is quite old so rebasing to the latest code could be challenging. Also @smustgrave wrote in #53 that he could not reproduce the issue.

I am removing the Novice tag from this issue because it looks like that the next step is to add tests and that isn't a novice task.

I’m using this documentation as a source: https://www.drupal.org/community/contributor-guide/task/triage-novice-is... →

I've merged the code that adds an option to createa a hash the current value.

For other enhancements for the Hash plugin, I've opened two new issues:
#3551693: Hash plugin: add an option for other hashing algorithms →
#3551694: Hash plugin: add an option to create a hash of a selected combination of values →

megachriz → created an issue.

megachriz → created an issue.

I checked and tested the code again. The test Tamper plugin that is defined using Attribute, is visible in the Feeds UI, the ECA UI and the Migrate Plus Feeds UI. So this works good. Merged.

I tested this again and in the following way:

• Configuring a single replacement
• Configuring two replacements
• By performing an import for content type with a list field, that has option "option_a", "option_b" and "option_c" and then configured the Tamper plugin to replace "a" with "option_a", "b" with "option_b" and "c" with "option_c". Imported a source that specified a, b and d for the list field. 'a' and 'b' got imported correctly and 'd' did not (which is expected).

All looks good, I triggered the merging process. :)

@andriy khomych
Thanks for your work on this issue, I think I'll look at this after DrupalCon Vienna (which I'm attending).

I merged the code!

Thanks for the code! I checked the ECA Tamper module and the plugins "Rewrite" and "Hash" (both requiring an item) are now no longer visible in the UI. I've merged 📌 Make Tamper plugins tell if they require a tamperable item Active meanwhile.

Tested this for the Rewrite plugin in combination with 🐛 Rewrite plugin doesn't replace "parent" values Active . Both together it looks to work well. Merged.

I tested this again manually and added some comments to the tests. I scheduled this to merge.

This is reported earlier and appears to be a bug in the Gin theme. See 🐛 When using Navigation module, Save button doesn't trigger Active .

Workaround is to disable "sticky action buttons" in the Gin theme settings.

I merged the code!

I would love to give this a look! But I think that will happen after DrupalCon Vienna (which I'm attending).

Good idea, but I think that requires a BC break, so that would go in a 4.x branch. Is there a way to update entities that are now of type "registration" to "rng_registration"?

It would be good to prefix all other entity types defined by RNG as well.