🇳🇱Netherlands @MegaChriz

Tagging with "Feeds", so all Feeds related issues can be found together that way.

Tagging with "Feeds", so all Feeds related issues can be found together that way.

I've updated the MR and I created an issue for Paragraphs: 📌 Remove usage of deprecated FeedsAnnotationFactory Needs review .

Note: 🐛 Adding a 'Paragraphs' field to a Feed's mapping throws error RTBC has tests for the Feeds integration, you might want to resolve that issue first.

MegaChriz → created an issue.

Tests have been added to 🐛 Adding a 'Paragraphs' field to a Feed's mapping throws error RTBC now.

MegaChriz → changed the visibility of the branch feeds_target_get_summary.3177867.3 to hidden.

@MegaChriz when you merge this, it might make sense to create a 4.x branch, and to tag a 4.0.0-alpha1 version when you add D11 support. This would allow you to keep adding changes to the 8.x-3.x branch in the case that something critical should happen, and to pivot Drupal 10 and 11 to fully use semantic versions.

@jcventura
Yeah, I would love to get semantic versioning for this reason, but a few people strongly adviced me to not create a new major version because that would slow down adoption of the new version + it creates an extra hurdle for people who want to upgrade to a new major Drupal version, Drupal 11 in this case.

For details, see https://drupal.slack.com/archives/C014CT1CN1M/p1659448352144959
See also https://medium.com/jakob-on-drupal/dont-go-making-major-version-changes-...

Anyway, I'm on holiday soon, so I'll probably not be able to commit this shortly - unless I've got so much bad weather on location that I just want to work on Feeds.

It looks like that some new phpcs issues are introduced with the code changes. We could consider to resolve 🐛 Fix PHPStan errors Needs work first, to also possible catch newly added phpstan issues.

Anyway, needs work. I think that at least tests must pass on Drupal 11 before merging it. Else there would be a risk to release something as D11 compatible while it may still have some incompatibility.

@KarlShea
The database update feeds_update_8007() caused an error on MySQL 8.0:

SQLSTATE[42000]: Syntax error or access violation: 1235 This version of MySQL doesn't yet support 'existing primary key drop without adding a new primary key. In @@sql_generate_invisible_primary_key=ON mode table should have a primary key. Please add a new primary key to be able to drop existing primary key.': ALTER TABLE "feeds_clean_list" DROP PRIMARY KEY; Array

It looks like to have something to do with the MySQL setting "sql_generate_invisible_primary_key". It is said that if that setting is turned off, the error should disappear.

Do you know if the database update could be rewritten in a way that it doesn't cause an error when the MySQL setting "sql_generate_invisible_primary_key" is turned on?

See 🐛 SQL error after upgrading to 8.x-3.0-beta5 Active

It appears to have something to do with a setting on MySQL, one that is called "sql_generate_invisible_primary_key". I read that if that is set to 0, then you should be good.
https://github.com/prisma/prisma/issues/20944
https://magento.stackexchange.com/questions/370387/mysql-error-mysql-doe...

I don't know how the issue should be fixed in an other way yet.

bbrala → credited MegaChriz → .

At the moment, Feeds can only import files to file fields that are coming from a http(s) url. So the input data for a file field should be something like "https://my_url/system/files/my_file_name.xlsx".

There is a bug when the autocreate option is turned on for files: #2973170: Importing new files fails when "Autocreate entity" option is turned on. →
There is a feature request to support local file paths as well for files: ✨ File target: add support for local file path as source Needs review

Does this answer your question? If not, feel free to reopen.

• I think that property_exists() should be used in both BaseItem::get() and BaseItem::set().
• BaseItem::get() should check if $this->data[$field] exists before returning that.
• BaseItem::toArray() should merge the object vars with $data array and then remove the extra data array from it.
• In Syndicati‎onItem.php‎ there is a blank line removed that should not have been removed.
• Note that the code is currently failing tests.

I got one issue when updating a site:

Data truncated for column 'entity_id' at row 789

feeds_import_log_entry.entity_id is allowed to be NULL.

Fixed that on commit.

Thanks for checking, @KarlShea! I merged the code.

Committed #2 with the following small changes:

• Updated code comments.
• Do not pass $existing_entity_id to the log message when skipping new entities because $existing_entity_id will be empty.

I merged the code!

I think I'll give this a go myself, so I can hopefully merge this for the upcoming release.

@KarlShea
I think it would be good to have one test focussed on Feeds Log, in feeds/modules/log/tests/Kernel. I think the other one could be added to the existing \Drupal\Tests\feeds\Kernel\UpdateNonExistentTest test class.

Thanks! I merged the code.

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

We could also perhaps disable (or rewrite?) some of the tests. For example, Feeds XPath Parser is an abandoned project, so that one is unlikely to get PHP 8 compatibility. It was replaced by Feeds Extensible Parsers almost a decade ago.

I also see the tests are using PHP 8.1. Would it work better if we use PHP 7.4 on GitLab CI instead? At least for now, so we have better hopes of passing tests. Tests fail on PHP 8 on Drupal CI as well, where the PHP 8.1 failures look related to Job Scheduler.

Hm, but it does fail tests. Is that perhaps caused by issues in modules that the D7 version of Feeds depends or integrates with? For example: Job Scheduler, Date, Organic Groups, Feeds XPath Parser...

@lelkneralfaro
Yes, I hope to create a new release this month. I originally planned to release in January this year, but other things got in the way. I'm now working on the last bits for the new release.

Alright, then only support Drush 12 and above it is. I'll leave that to somebody else to get that done (at least for now), so I can focus on getting the last bits done for the upcoming release.

I haven't paid attention to the Drush integration, but I think we only would need to support Drush 12 and above? Or maybe Drush 11 too? At least we should be compatible with the latest Drush.

@ankitv18
I'd like to create one last Drupal 9 compatible release of Feeds (hopefully next week, I'm already busy writing the release notes and see what I want to commit last minute) and then after that Drupal 9 support can be dropped so focus can be on getting Feeds compatible with Drupal 11.

Almost. BaseItem should check first if the requested field exists as property in the class. And if not, fallback to the $data array.

Sounds reasonable to add a config option like that. I think that a global configuration option for this would make the most sense? That means we need a userprotect.settings config object and a form to set the configuration. And of course the set configuration needs to be respected in userprotect_form_user_form_alter().

Thanks for the review! I merged the code.

'parent:fid' is actually a dynamic property. It is generated in \Drupal\feeds\Feeds\Source\BasicFieldSource. Besides 'parent:fid', you also have 'parent:uid', 'parent:created', 'parent:title' and many others.

So simply declaring $parent_fid as a variable here is not a sustainable solution.

So I think that BaseItem needs to check in methods `get()` and `set()` if the property to set exists. And if not, write to `$data` property like DynamicItem does. I don't know however if there any drawbacks in that approach, but I think the risk for that is low.

Thanks for fixing this! I merged the code.

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

@nicxvan
Thanks for taking a look!

It looks like that Queue Unique requires a manual intervention in settings.php.

Anyway, I think double queuing is prevented. Because as soon as a mail for an order gets queued, the order gets flagged as queued in the "commerce_abandoned_carts" table. And when selecting abandoned carts, orders that have already been queued are filtered out.

Code that flags an order as queued, from \Drupal\commerce_abandoned_carts\AbandonedCarts::queueSendMails():

// If not in test mode, flag that for this order a mail has been queued.
if (!$this->config->get('testmode')) {
  $this->connection->merge('commerce_abandoned_carts')
    ->keys(['order_id' => $record->order_id])
    ->fields([
      'order_id' => $record->order_id,
      'status' => static::QUEUED,
      'timestamp' => $this->time->getRequestTime(),
    ])
    ->execute();
}

Code that makes sure that already queued orders are not selected, from \Drupal\commerce_abandoned_carts\AbandonedCarts::getAbandonedOrders():

$select->leftJoin('commerce_abandoned_carts', 'a', 'o.order_id = a.order_id');
  // (...)
  ->isNull('a.status')

So the only way double queuing could theoretically happen (when not in test mode):

• A module that somehow can act on a merge query that gets executed - but generates a fatal error, causing the query to not get executed.
• The server gets shut down exactly at the time that the queue item got created, but the merge query did not get executed yet (extremely unlikely).

Double queuing does happen when operating in test mode, but I think that's correct. Because you are still testing then what happens.

If you'd like this plugin to become officially part of Tamper, it needs config schema, a unit test and a functional test.

Perhaps Entity Clone can clone feed types too. For that, you would need to try the module. :)

Tests are passing! Merged.

I merged the code!

I removed the debug and theme function. Let's see what the tests think of that.

Okay, I had not much to do this evening, so I think that I fixed the issues in the UI. And I added test coverage that covers setting the source encoding in the UI and that covers DefaultController::encodingAutocomplete().

Let's see if tests pass.

I worked on this a bit.

I found out this is actually about bringing the UI for setting the source encoding back:

It needs work because:

1. Saving the configuration fails.
2. TextEncoder::configFormValidate() doesn't get called yet.
3. A test is needed to cover interaction with the page at /_feeds_ex/encoding_autocomplete. Kernel test?
4. A test is needed to make sure that the source encoding setting can be configured in the UI. Functional or FunctionalJavascript test (or both).

Not sure if I get to that soon.

I merged the code!

It looks like it is passing tests again! It doesn't fix all PHPStan issues yet, but some can indeed be handled in one of the related issues.

Just waiting to see if tests pass on D9 too.

I think in order to get phpstan be passing, it would be good to remove the dead code. And then perhaps create a new issue to re-add the debug feature from the D7 version - if we still want it. Maybe we don't need it per se?

I think it would be good to create a new release of Feeds Extensible Parsers soon and then drop Drupal 9 support shortly after that.

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

MegaChriz → changed the visibility of the branch phpstan to active.

MegaChriz → changed the visibility of the branch phpstan to hidden.

I'm kind of confused what this issue is fixing. The property of "Inner XML" is already part of the XML Source:

Am I missing something or should this be closed as "Works as designed"?

Note that the feature in the image wasn't there yet at the time that this issue was opened.

It would be useful to see when this exactly fails. It looks like it doesn't occur when there is a syntax error in the JSON, because then there's a RuntimeException. (This error message now includes the JSON string, which was added in ✨ Add a feature to display JSON errors Fixed ).

This needs test coverage. The test should check if the feed name and the file path are getting displayed. Perhaps it needs to be a Kernel test if it turns out to be hard to fit it in an Unit test.

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

I rebased the code to see if there were any phpcs errors and luckily there aren't.

I also wondered if instead of displaying the whole JSON, if perhaps the error message could be more specific on where the JSON is wrong, but not sure if there is an easy way to display that. json_last_error_msg() does not give any useful info on a syntax error at least.

So I merged the code. We could open a follow-up if you have ideas on how the JSON error could perhaps be more specific.

I merged the code!

I've just added phpcs:ignore lines to ignore the remaining phpcs issues. In some cases it is allowed to pass a PHP variable to t(). The core module 'Database Log' does it too. For example in dblog.admin.inc: t($type) and in DbLogController: $this->t($dblog->type).

The commented out code is kept for now, but in the comment above I added more information about why the commented out code is there. And what the next step is for possibly resolving it.

Merged the code!

I removed the usage of the class property $feedType in the test FeedTypeEditFormTest, because I didn't see really the benefit of using a class property here. In some tests this is used to create a feed type that is then use in multiple tests. But here we only have one test.

I also updated the GitLab CI file using the latest template of https://git.drupalcode.org/project/gitlab_templates/-/blob/main/gitlab-c....

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

I merged the code!

Removing the help text to avoid confusion I think is better for now.

Ideally, maybe JsonSource shouldn't extend BlankSource, because it could possibly create similar issues in the future should other changes happen in BlankSource. But for that, I think there should first be another base class in Feeds that does most of what BlankSource does, except setting a description that's really only relevant for BlankSource.

I reduced the scope of the issue this is fixing. This reduces the chance of regressions, which was I afraid for, but it’s also possible that doesn’t fix the reported issue entirely.

Now an import gets aborted only when the file to import (which is saved in the "in_progress" directory) no longer exists. When Feeds fails to fetch a resource, for example because of a 404, Feeds will not abort the import process on cron runs. Instead, Feeds will retry the fetch on the next cron. It does so forever, which is an issue as well, but I think we should address that in 🐛 Feed fetch errors don't seem to be handled gracefully Active instead.
By doing so, we can at least get a part of the issue fixed and at the same time reduce the chance of regressions.

To make the chance of regressions smaller, I've decided to make the scope of 🐛 FetcherResult::checkFile() throws \RuntimeException, which is never caught Needs review smaller. The fix in that issue now no longer aborts the import process on cron runs if fetching a source fails with a 404. Instead, Feeds will retry over and over again. That's not great, but I don't want imports to fail that experience a temporary hiccup. In such cases, I think it would be good if Feeds retries. Though Feeds should not attempt to retry forever as it is now. So this issue therefore still is a valid issue.

The issue does need an issue summary update, because some side effects reported in it are no longer true:

• When a feed gets unlocked, any queue items related to that feed now get removed first. So multiple imports for the same feed happening simultaneously should no longer happen.
• Feeds no longer unlocks a feed after 12 hours are passed. It unlocks it only if Feeds thinks the import no longer runs (by checking the number of items on the queue). If it detects that an import could still be running, the lock gets extended instead. And a warning about the lock getting extended gets logged.

Again, what still is an issue is that Feeds would try to fetch a resource that results into 404 forever, once on every cron run. And we should somehow limit the number of retries. It attempted to do that in 📌 Add DelayedRequeueException feature to feeds queue - D9.1 Active , but that became quite complicated.

It sounds like that your issue can be resolved with the "Rewrite" plugin. You can then type in something like 372145[field].

If you need more advanced transformations of data, you could use an event subscriber in a custom module that acts on the parse event. The process of how to do that is described on https://www.drupal.org/docs/contributed-modules/feeds/feeds-howtos/alter... →

It is discouraged to have PHP code in the database or in config files as that could have a potential security risk. That being said, in some situations it could be practical, for example when you are tight on a deadline. But I think I don't want such plugin to exist in the Tamper module itself, for security reasons. For D7, this plugin was also in a separate project: https://www.drupal.org/project/feeds_tamper_php → . You could ask there if they want to port the project to modern Drupal, if you find that you more often would need it.

Cool. Thanks for testing! I merged the code.

@ptmkenny
A config entity doesn't always have an 'id' property, but it does have an 'uuid' property:

config_entity:
  type: mapping
  mapping:
    uuid:
      type: uuid
      label: 'UUID'
    langcode:
      type: langcode
    status:
      type: boolean
      label: 'Status'
    dependencies:
      type: config_dependencies
      label: 'Dependencies'
    third_party_settings:
      type: sequence
      label: 'Third party settings'
      sequence:
        type: '[%parent.%parent.%type].third_party.[%key]'
    _core:
      type: _core_config_info

So I reverted your changes in the MR.

From https://www.drupal.org/node/3349638: →

Note: There's no disruption for config schemas switching from type: label to type: required_label, because config schema validation is not yet being used.

Hm, that's not true for Feeds!

Besides machine_name, other newly introduced types don't seem to be significant for Feeds: https://www.drupal.org/list-changes/drupal/published?keywords_descriptio... →

The issue is actually that Drupal 10 apparently added some new data types for config schema. Data types 'machine_name' and 'required_label' are new.

Config schema for user role in Drupal 9:

user.role.*:
  type: config_entity
  label: 'User role settings'
  mapping:
    id:
      type: string
      label: 'ID'
    label:
      type: label
      label: 'Label'
    weight:
      type: integer
      label: 'User role weight'
    is_admin:
      type: boolean
      label: 'User is admin'
    permissions:
      type: sequence
      label: 'Permissions'
      orderby: value
      sequence:
        type: string
        label: 'Permission'

And in Drupal 10:

user.role.*:
  type: config_entity
  label: 'User role settings'
  mapping:
    id:
      type: machine_name
      label: 'ID'
    label:
      type: required_label
      label: 'Label'
    weight:
      type: integer
      label: 'User role weight'
    is_admin:
      type: boolean
      label: 'User is admin'
    permissions:
      type: sequence
      label: 'Permissions'
      orderby: value
      sequence:
        type: string
        label: 'Permission'

So changes in Drupal core broke this in Feeds.

Now that 🐛 in_progress filesystem grows indefinitely Active and 🐛 Delete Empty Files Created If Empty/Error Exception is Returned Needs review are done, I could perhaps give this issue another look.

It would be good to have an automated test that tests importing and logging entities with a non-numerical ID. In core, in the test module "entity_test", there is a content entity type called "entity_test_string_id" (\Drupal\entity_test\Entity\EntityTestStringId). That entity type would probably be suitable for such a test. Do you want to write tests for this issue, @KarlShea?

All tests are passing! Merged the code!

I see that the reported issue can technically still happen in case of a 304, but it seems only to happen in case fetching happens 'stand-alone', separate from the import process. Fixing that will be done in 🐛 Delete Empty Files Created If Empty/Error Exception is Returned Needs review .

Okay, I merged this. And retested the scenario. When a resource that Feeds tries to fetch returns a 404, Feeds will keep trying on every cron run to fetch it over and over again. But with the changes from this issue it no longer stores a file on the file system.

It has been awhile for me, but I think that the other issue (Feeds endlessly tries to fetch a resource) should be handled in 🐛 FetcherResult::checkFile() throws \RuntimeException, which is never caught Needs review .

And with this change in, we can plan a new Feeds release: the last Feeds release with Drupal 9 support.

@ptmkenny

With logging enabled, when both "Log message" and "Log item" are checked, if you uncheck "Log message" first, then "Log item" is disabled and you cannot uncheck it.

But after you save the settings, "Log item" will be unchecked. The idea is that if you disable logging for a certain operation, you cannot log items for it. So that's why the "Log item" checkbox will become disabled.
I do see that there's an issue that when both checkboxes are not checked on page load, when you enable logging for a certain operation, the "Log item" checkbox remains disabled. But when unchecking and checking logging for an operation, you can control "Log item" again.

I think changing the form for the log settings is something that would better be addressed in a follow-up issue, but thanks for testing @ptmkenny :)

I merged the code.

MegaChriz → created an issue.

Tests are passing now! Merged.

Though I think I may accidentally have fixed the majority of this issue in 🐛 Removing process plugins for multiple properties at once does not work Active instead.

Alright, the issue looks like to be fixed and tests are passing! Merged.

@adstokoe
It's bad UX, but it doesn't break functionality. And I think it's a hard one to fix. But because it doesn't break functionality, this issue is quite low on my priority list. My priority right now is first review and commit things that others have worked on recently. Then finish 🐛 in_progress filesystem grows indefinitely Active and create a new release and then get Feeds and related modules Drupal 11 compatible. And after that hopefully I'm able to resolve the remaining stable release blockers to finally create an official stable release. And after all that, then there might be room for me to address this issue. Though there also enough other issues that I think would have higher priority than this one. These are listed on 🌱 [meta] Feeds 8.x roadmap Active .

So in other words, I don't see this issue getting addressed in the nearby future and probably not in the next two years. At least not by me.

I cannot reproduce this issue. Note that this feature is also covered by automated tests. So perhaps it doesn't work correctly in a certain configuration.

Can you provide the following:

• The source file you were trying to import (may also be a sample file, as long as that sample file can demonstrate the problem).
• An export of your configuration, packed inside a feature → module, which includes:
  • The Feed type;
  • The content type selected on the processor;
  • Fields used on your content type;
  • Field storage items of your fields;
  • Optionally other configuration that is related.

If you want to be sure that I can install your configuration, try to enable the created feature module on a clean install.

I finally merged this one!

Thanks for helping! Unfortunately, the JS in modules/log/js/feed_type_settings.js is no longer doing what it should do.

In the vertical tab 'Log settings' on a feed type page it should tell the user whether or not logging is enabled for the feed type:

After the changes from this issue, the summary on the vertical tab 'Log settings' always says that logging is disabled, even if it is enabled.

Since in ✨ Use queue for sending emails Needs review the sent messages are logged on the order (by using the commerce log template), we no longer need this change.

For the 8.x-1.x branch I think it isn't worth to try to make that Drupal 11 compatible.

Committed #2.

The latest code in ✨ Use queue for sending emails Needs review has all coding standards fixed. I think it would be a waste of energy to try to fix it also for 2.0.x.

The settings are removed in ✨ Use queue for sending emails Needs review .

I think this is fixed? Otherwise, it might be in ✨ Use queue for sending emails Needs review .

This is being worked on in ✨ Use queue for sending emails Needs review .

Here is a patch written on top of ✨ Use queue for sending emails Needs review . Since over there I'm working on a major overhaul of the module, it makes no sense to try to fix this for 2.0.x. I'd like to set the version to 2.1.x, but I'm not able to create a dev release for that branch yet.

This has been fixed in 🐛 Orders with multiple order items generate multiple emails RTBC .

This has been fixed in 🐛 Class "Drupal\Component\Utility\Mail" not found RTBC .

The MR is merged!

I found out that the SQL error did occur, but it does not get reported by the test. Probably because the cron runner catches and logs that exception, so the test does not see it.

I've updated MR 4 (3366213-groupby-ordder) with the following changes:

• The test from the tests_only branch is added.
• The logged message is no longer a translation (addresses #8).
• An additional fix for 🐛 SQL error if we have orders with multiple line items Needs review is included, a merge query is used instead of an issert query. This avoids the SQL error.

If this passes tests, then I think it's ready.

@nicxvan
Note: ✨ Use queue for sending emails Needs review is not yet updated. Good idea to expand the test by checking the number of queue items created there. However, if you add the same task twice to a queue, I think two mails will still be sent, because the one queue item does not know about the existance of the other queue item.

I've worked on test coverage for this issue. It does not cover the SQL error yet that is reported in 🐛 SQL error if we have orders with multiple line items Needs review .

I also haven't checked yet if the provided fix makes the test pass. I only had a few spare minutes to write a test today.