🇳🇱Netherlands @megachriz

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.

megachriz → created an issue.

This looks like to be already fixed in the dev version.

I plan to create a new release after 🐛 Rewrite plugin doesn't replace "parent" values Active is reviewed and fixed.

Thanks for reporting and fixing!

The merge option on drupal.org seems to be gone, but I've triggered a merge on GitLab. Apparently, merging does not happen immediately there. So I hope I've done merging the right way.

Unfortunately, my client has postponed upgrading their sites to Drupal 11. There is no new date planned for it yet. So it probably will take a while longer until I can get back to this project.

Thanks for the patch! I turned it into a MR so tests can run. I also fixed a few other PHP 8.4 related issues that were reported by PHPStan.

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

@_cosmos_
Can you test/review 🐛 Multiline Find/Replace Adds Carriage Return Active ? I think that I've fixed the issue there. :)

I've addressed all remarks of mine from #9.

Most important changes:

• The http_headers setting is now saved as array instead of a string. So if you had used a previous diff, you would need to reconfigure your feed types/feeds. I do have made it so that the forms still work if the setting was saved as string.
• There is now test coverage for this feature.

@skaught
Thanks for helping, could the improvements be added to 2928904-media-target as well? Or do the changes from #127 not build on top of what is in 2928904-media-target? I see that the branch '2928904-127-feeds-combined-media-fix' is based on a very old Feeds version (8.x-3.0-alpha11, almost 4 years ago).

I have planned to look more closely to this issue in September/October this year. (Though I've also a few projects left to make Drupal 11 compatible, which I've also planned for that period.)

I see this is still an issue. It looks like that can only experience this issue when you configured the "Feeds item" to be displayed on an imported entity with the formatter set to "URL of the feed item" (the default formatter). By default, the "Feeds item" field is hidden, so that's why I did not noticed this issue myself before.

I expect that the provided MR fixes the issue. Hopefully all tests pass.

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

I quickly scanned the code of EntityProcessorBase in the web interface and cannot find that second account switch right now. But I was pretty sure that I had made that. Did I perhaps only implement that for the D7 version of Feeds? 🤔

If I'm not mistaken, Feeds does a second account switch (to the owner of the item) when you enable the "Authorize" option. I'm on mobile right now, so it's harder to check for me now if that is the exact option that triggered the second account switch, but at least there exists a second account switch in the code.

Thanks for the review! I merged the code.

I think that adding Attribute does require Drupal 10.2?

Because \Drupal\tamper\Attribute\Tamper extends \Drupal\Component\Plugin\Attribute\Plugin. The latter class is not available in Drupal versions before 10.2.

I think that adding Attribute does require Drupal 10.2? Anyway, the Attribute code could be updated in 📌 Add attributes alongside annotations Postponed , though there is a good chance they would need to be updated again when 📌 Make Tamper plugins tell if they require a tamperable item Active gets merged.

@ptmkenny
If you like and have the time, there are a bunch of Tamper issues updated that can be reviewed/tested. :)
I keep track of issues that I work on in a spreadsheet: https://docs.google.com/spreadsheets/d/1cn4vmHosTfxgyI0zlvVv_ce94715eH-m...

megachriz → changed the visibility of the branch attributes to hidden.

I think it is nice if we can add support for attributes now, so I'm reopening this. I do think of dropping Drupal 9 support after the next release.
Also because for Drupal 12 it will be required for plugin types to support attributes: https://www.drupal.org/node/3522776 → .

@kopeboy
I tried the eca_tamper module and I see that the Hash plugin indeed does nothing there. That's because eca_tamper doesn't pass a tamperable item and currently the Hash plugin cannot work without an item.

So in the MR I provided an option to hash the input value instead of the item. And if no item is passed (in case of eca_tamper), the plugin will fallback to the input value as the data to hash.

Other hash options

I like the addition of other hash options that are provided in the code from #9, but I found out that hash_hmac('md5', 'Foo', ''); does not provide the same result as md5('Foo');, so that change would break backwards compatibility. Because of the amount of complexitity these options would add, I left this out for now.

Creating a hash from other fields on the item

Proposed in #6. This would be a nice option to add too, but I left that also out for now.

megachriz → created an issue.

I've added the fallback option. 🙂

I experience only a single issue with this plugin and that one is handled in 🐛 Multiline Find/Replace Adds Carriage Return Active , so I'm setting this issue back to fixed. If you still get other errors while using this plugin, maybe it's better to handle that in a new issue.

It looks like this use case would fit better in the Rewrite plugin. There's a feature request to add an option to the Rewrite plugin to only rewrite if empty: ✨ Rewrite value using another field value only if empty. Needs work

Recently, also a Twig plugin was added (to the dev version as of now), so you could also use that one for conditionally setting values too.

We already have the "Trim" plugin that could be applied after the "Explode" plugin.

Tamper has been running on Drupal 11 for a while, so I think that this is fixed.

I provided a simple solution + test coverage: submitConfigurationForm() just replaces "\r" with an empty string before the text is converted to an array.

After this issue was posted, some changes were made to the boolean plugin in #3322750: Some plugins do not implement submitConfigurationForm() correctly - Add UI tests → .

I found one case where the plugin did not work as expected: when the "If no match" was set to "True". In this case the value was returned as is instead of a TRUE value. That's because PHP evaluates the following condition to true when $this->getSetting(self::SETTING_NO_MATCH) is TRUE:

$this->getSetting(self::SETTING_NO_MATCH) == 'pass'

And this resulted into the original value being returned instead of TRUE.

Changing this to

$this->getSetting(self::SETTING_NO_MATCH) === 'pass'

fixes that issue.

I have reworked the tests to find this bug. I believe the issue reported originally in this issue got fixed by the changes in #3322750: Some plugins do not implement submitConfigurationForm() correctly - Add UI tests → .

@thomasmurphy
I could not reproduce your issue. When I set the "If no match" to "Null" and I import something that does not match the "Truth" or "False" setting, I see in the Feeds log that the item's field really equals null:

{"title":"Nothing","bool":null}

So I assume the value gets modified later in the process. Either by a FeedsTarget plugin or by the field that the data gets written to.

I've provided a solution for this issue! Even though the tests are not passing yet, this is now ready for review and testing.

It works as follows:

1. Tamper plugins that work with a tamperable item (like the Rewrite plugin), provide a list of source fields they use on the item.
2. Feeds Tamper checks if any of these source fields are provided by FeedsSource plugins. This is the case for all "Feed entity" sources, that are listed for the Rewrite plugin as "[parent:*]" tokens.
3. Feeds Tamper loads the data for FeedsSource source fields.

Be sure to test it in combination with 📌 Make Tamper plugins tell which properties they use of a tamperable item Active . That issue provides changes for the Tamper module, so that plugins can provide a list of source fields that they use. Changes don't apply on the latest Tamper release, so you should apply them on the dev release. The Tamper dev release also contains other improvements for the Rewrite plugin.

megachriz → created an issue.

This sounds like an UX issue in Feeds Tamper, so moving it to that project.

The way Feeds Tamper currently works:

1. It sorts all Tamper plugins by weight.
2. Then it loops through all Tampers and groups them by source field.
3. It loops through all source fields with Tamper plugins.
4. Per source field, it applies all Tamper plugins for that source field.

So I think that the issue is is that the tampers for source 'Y' are handled later than those for source 'X'. This means that when Feeds Tamper applies tampers for source 'X', the value for the temporary target is not yet available.

A short term solution would be to set the weight of the first Tamper plugin for source 'Y' to a lower value than for source 'X'.

A long term solution would be to redesign the Feeds Tamper UI, so it becomes clear which source is taken care of first. Even better would be to change the processing logic so that tampers are not executed strictly per source in sequence. Instead, tampers from different sources could be interleaved according to their weights, allowing you to apply a tamper to source X, then one to source Y, and then another to source X within the same run. But that change has more impact and could cause BC breaks.

By the way, sorry for the very late reply. I'm walking through the bug reports for Tamper to see what should be fixed for a stable release.

I merged the code!

I merged the code!

@codebymikey
Thanks for your review! I did not notice it earlier, since the issue did not get updated.

I merged the code!

I have been using this plugin for a while and did not detect new issues with it. Code looks good too. Merged!

Alright, I tested the code on a D11 instance and on a D10 instance. It looks good to go. Merged!

megachriz → created an issue.

@calbasi
It would be good to also check for extra spaces on the definition of the CSV sources, on the "Custom sources" tab. Especially check if there are extra spaces in the "Value" field of a CSV source. Extra spaces for "Label" do no matter.

"Custom sources" tab:

Check "Value" field of a CSV source:

@calbasi
When I inspect the text that you posted I see:

codi_sigpac [space] [tab] codi_sigpac

So can it be that you added an extra space when you defined the source? codi_sigpac != codi_sigpac.

In 🐛 CSV headers with extra space don't get mapped to correct fields Needs work there is being worked on trimming CSV column names (during parsing and during entering column names), so that accidentally additional added spaces don't cause that the CSV column names do not match.

I plan to work on this in the next few weeks.

This was already reported before in 🐛 Rewrite plugin does not work after Explode Closed: duplicate . However, I just closed that one as a duplicate too because a fix for this issue is now available in a combination of two other issues:

• 🐛 "Rewrite" plugin doesn't work right Active
  This ensures that the data of previously applied Tamper plugins (thus before the Rewrite plugin) is written back to the item. The Rewrite plugin then finds the most up to date data on the item.
• ✨ Rewrite plugin: iterate through values to rewrite Needs work
  Among other things, this adds support for arrays for the Rewrite plugin. If your initial source value is for example Foo,Bar,Qux and you explode that using a comma as separator and then use the Rewrite plugin to add a "x" to it, you'll get ["Foox","Barx","Quxx"] instead of ["Foox","Foox","Foox"].

A fix for this issue is now available in a combination of two other issues:

• 🐛 "Rewrite" plugin doesn't work right Active
  This ensures that the data of previously applied Tamper plugins (thus before the Rewrite plugin) is written back to the item. The Rewrite plugin then finds the most up to data on the item.
• ✨ Rewrite plugin: iterate through values to rewrite Needs work
  Among other things, this adds support for arrays for the Rewrite plugin. If your initial source value is for example Foo,Bar,Qux and you explode that using a comma as separator and then use the Rewrite plugin to add a "x" to it, you'll get ["Foox","Barx","Quxx"] instead of ["Foox","Foox","Foox"].

Closing this as a duplicate now.

I've provided a fix and I've updated the issue summary to clarify what the problem is.

Moving this to Feeds Tamper now.

@starlight-sparkle
Thanks for your contribution! Can you put your changes in a MR? The testbot on drupal.org no longer looks at patches, so changes need to be in a MR now if you want that the code eventually gets added to the module.