Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Coming from #2708693: Validation failure for body text filter format due to lack of permission
We are currently switching to the feed owner during cron, but we should also switch during batch import. This will cause less surprises when cron finally runs.
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff-2811429-15-20.txt | 1.43 KB | MegaChriz |
#20 | feeds-switch-owner-ui-2811429-20.patch | 97.34 KB | MegaChriz |
Comments
Comment #2
madelyncruz CreditAttribution: madelyncruz commentedI'm also having issue like this. It would be nice if there's a solution/patch for this.
Comment #3
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedI'm working on this issue, but it involves a lot of changes. Work in progress in patch attached. Not functional yet.
Comment #4
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedThis big change introduces the following:
In a follow-up
FeedsImportHandler::import()
andFeedsImportHandler::pushImport()
should also pass their logic to an executable, so that these too follow the same logic as batch imports and cron imports. Right now, no account switch happens in there.Let's see which tests are failing after this refactoring. I'm also not sure yet if it already fixes the reported issue.
Comment #5
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedHere is also a test-only patch that should demonstrate the issue and thus fail.
Comment #8
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedOkay, it looks like
FeedImportHandler::import()
andFeedImportHandler::pushImport()
needs to be updated in this issue as well.Let's fix some coding standard issues first.
Comment #9
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedLet's see what this does. I'm not sure about the implementation of
pushImport()
yet. I'm also not sure if the implementation ofFeedsExecutable::handleException()
is correct.Comment #10
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedApparently, the parse state can be explicitly set to
NULL
, which happens inFeedsExecutable::doFetch()
:This is taken from
FeedRefresh::doFetch()
. I assume this is because the parse state needs to be reset after a second fetch.Comment #11
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedThis updates the unit tests FeedRefreshTest and FeedImportHandlerTest. It also adds an unit test for the class FeedsExecutable.
The test
FieldValidationTest::testImportFieldWithAdminFilterFormatInUi()
which was still failing has been corrected. The message after import is not "Created 2 nodes", but "Created 2 Articles". Because this test was updated, I also made a new test-only patch, to ensure this whole refactoring thing actually fixes an issue.Points of #9 still need to be addressed.
Comment #12
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedI've spinned off the issue I found and fixed in #10 in #3086342: $feed->getState() can return NULL while it should always return an instance of StateInterface.
Besides this patch being a reroll because of that, this also fixes coding standard issues and corrects
FeedsExecutable::handleException()
(I checked how the previously existingFeedImportHandler::handleException()
was implemented).Needs at least kernel test coverage for
FeedImportHandler::pushImport()
. Perhaps I'll create a spin-off issue for this as well.Comment #13
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedTwo changes:
FeedImportHandler::pushImport()
.::handleException()
so that it rethrows \RuntimeException instances. I'm not sure though why normally these exceptions should be catched.Comment #14
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedReroll, needed after fixing several deprecations in dev.
Comment #15
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedUsing custom code that depends on these changes, I saw a need to have access to
$params
(relevant to the current stage) inFeedsExecutable::handleException()
, so I'm adding extra context to this method.Comment #16
Thangaraj MoorthiHi,
I used,
drupal 8.6.16
Feeds 8.x-3.x-dev
Feeds_Tamper 8.x-2.x-dev
Before applying the patch, the testing was passed
After applying the patch https://www.drupal.org/files/issues/2019-11-10/feeds-switch-owner-ui-281...
The testing gets success.
Thanks.
Comment #17
MegaChriz CreditAttribution: MegaChriz as a volunteer commented@thangaraj.moorthi
Thanks for testing! Unfortunately, running the automated tests locally give no new information. Automated tests are run in isolation, thus with only Feeds enabled. So your test results only match with the test results from the testbot, which also reported success.
The most useful thing to test here is testing doing imports through the UI manually and in combination with several contributed projects that depend or integrate with Feeds, thus with as much projects that are listed on https://www.drupal.org/project/feeds/ecosystem (that have D8 versions). The purpose of this testing is to catch regressions: features provided by these contrib projects that worked with alpha6, but get broken after this patch is applied.
I’m testing this patch too on a live site, combined with a bunch of custom modules that integrate with Feeds. So far, any reports from my client don’t seem to be related to this patch.
Comment #18
Thangaraj MoorthiWell Ok, Thank you @MegaChriz
Comment #19
Thangaraj MoorthiHi,
Before applying the patch, I tested it manually.
I had with more custom modules ie) Feeds_Tamper, Feeds_Personify, Commerce_feeds..
I'm tested on feeds tamper functionality and the import process is gets done successfully.
I used,
drupal 8.7.10
Feeds 8.x-3.x-dev
Feeds_Tamper 8.x-2.x-dev
I have to do some more testing.
will update that soon.
Thanks
Comment #20
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedWhile working on #3069752: Entities sometimes get removed/unpublished unexpectedly on cron I noticed some tests did not pass on Drupal 8.8 with this patch installed. Hopefully with the attached patch they do.
Also catched a deprecated call to
getMock()
.No functional changes were made. Only test fixes.
Comment #21
Thangaraj Moorthi@MegaChriz
I used drupal 8.8.0-rc1,
feeds 8.x-3.x-dev
I'm not getting any getMock() deprecated errors, but having some other deprecations even after applying the patch.
Let me know if you need any clarification.
Comment #22
MegaChriz CreditAttribution: MegaChriz as a volunteer commented@Thangaraj Moorthi
It's normal that deprecation warnings for Drupal 8.8 are there. Feeds will support Drupal 8.7 for at least a few months, so D8.8 deprecations cannot be fixed right now. All warnings look unrelated to the patch, even the one that can be fixed right now (about deprecated constant REQUEST_TIME). Deprecation fixes can be handled in: #3042774: Drupal 9 Deprecated Code Report.
Comment #23
Thangaraj MoorthiOk Thank you @MegaChriz
Comment #24
jamesdixon CreditAttribution: jamesdixon as a volunteer and at Dialed In Design commentedThanks for your work on this guys.
Looks like @Thangaraj Moorthi was able to test this one out and it worked for him outside of the Drupal 8.8 deprecations which we aren't worrying about now.
Is this sufficiently reviewed or is there more testing and review needed?
Comment #25
MegaChriz CreditAttribution: MegaChriz as a volunteer commented@jamesdixon
I asked @damondt if he would check if his contributed Feeds modules still work and he hopes to try it tomorrow. From Slack, today:
I think especially his contributed module Feeds Halt Imports could be influenced by this patch (based on the name of the module, I haven't tried the module myself yet). On the other hand, when briefly expecting its code I don't see right away something that would potentially break.
Comment #26
jamesdixon CreditAttribution: jamesdixon as a volunteer and at Dialed In Design commentedCool, lets see how @damondt's tests go.
Comment #28
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedI have this patch running on several live sites for about two months now and did not encounter any issues that are related to this patch. I also walked through all the code one more time to see if I perhaps could find something that could have impact. I only found some spelling errors or small mistakes in a code comments.
Committed #20 with a few changes in interface texts and code comments.
Comment #29
MegaChriz CreditAttribution: MegaChriz as a volunteer commented