On editing feed item field of a content type we get below error:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "hidden" plugin does not exist. in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 52 of /docroot/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

Installation specification:
name: Feeds
description: 'Aggregates RSS/Atom/RDF feeds, imports CSV files and more.'
package: Feeds
type: module
core: 8.x
version: 8.0-dev
configure: feeds.overview_types

Also in add field of content type we cannot see option to add feed item field. We have created one feed importer using feeds module on our site.

CommentFileSizeAuthor
#113 Screen Shot 2023-09-25 at 11.27.59 AM.png584.53 KBjnicola
#113 Screen Shot 2023-09-25 at 11.27.01 AM.png72.13 KBjnicola
#108 interdiff-2799225-107-108.txt20.25 KBmegachriz
#108 feeds-feeds-item-formatters-2799225-108.patch34.47 KBmegachriz
#107 interdiff_105-107.txt2.25 KBjamesdixon
#107 feeds-feeds-item-formatters-2799225-107.patch33.69 KBjamesdixon
#105 interdiff-2799225-102-105.txt2.24 KBmegachriz
#105 feeds-feeds-item-formatters-2799225-105.patch33.84 KBmegachriz
#102 interdiff-2799225-101-102.txt1.55 KBmegachriz
#102 feeds-feeds-item-formatters-2799225-102.patch33.45 KBmegachriz
#101 interdiff-2799225-086-101.txt5.42 KBmegachriz
#101 feeds-feeds-item-formatters-2799225-101.patch33.59 KBmegachriz
#86 interdiff_85-86.txt3.07 KBjamesdixon
#86 feeds-feeds-item-formatters-2799225-86.patch34.36 KBjamesdixon
#85 interdiff_81-85.txt1.89 KBjamesdixon
#85 feeds-feeds-item-formatters-2799225-85.patch33.43 KBjamesdixon
#83 text-formats.png21.02 KBmegachriz
#83 field-edit.png73.53 KBmegachriz
#81 feeds-feeds-item-formatters-2799225-81.patch33.44 KBjamesdixon
#81 interdiff_80-81.txt1.59 KBjamesdixon
#80 feeds-feeds-item-formatters-2799225-80.patch33.3 KBjamesdixon
#79 interdiff_76-79.txt1.94 KBjamesdixon
#79 feeds-feeds-item-formatters-2799225-79.patch33.62 KBjamesdixon
#77 interdiff_74-76.txt5.92 KBjamesdixon
#77 feeds-feeds-item-formatters-2799225-76.patch32.93 KBjamesdixon
#74 feeds-feeds-item-formatters-2799225-74.patch32.05 KBjamesdixon
#73 interdiff_71-73.txt1.1 KBjamesdixon
#73 feeds-feeds-item-formatters-2799225-73.patch32.36 KBjamesdixon
#71 interdiff_68-71.txt3.76 KBjamesdixon
#71 feeds-feeds-item-formatters-2799225-71.patch32.37 KBjamesdixon
#68 interdiff_66-68.txt4.9 KBjamesdixon
#68 feeds-feeds-item-formatters-2799225-68.patch32 KBjamesdixon
#66 interdiff-2799225-65-66.txt10.17 KBmegachriz
#66 feeds-feeds-item-formatters-2799225-66.patch26.7 KBmegachriz
#65 interdiff-2799225-63-65.txt1.71 KBmegachriz
#65 feeds-feeds-item-formatters-2799225-65.patch25.13 KBmegachriz
#63 interdiff-2799225-61-63.txt5.38 KBmegachriz
#63 feeds-feeds-item-formatters-2799225-63.patch25.08 KBmegachriz
#61 interdiff-2799225-59-61.txt3.62 KBmegachriz
#61 feeds-feeds-item-formatters-2799225-61.patch24.93 KBmegachriz
#59 interdiff-2799225-58-59.txt10.62 KBmegachriz
#59 feeds-feeds-item-formatters-2799225-59.patch25.07 KBmegachriz
#58 interdiff_55-58.txt4.91 KBjamesdixon
#58 feeds-feeds-item-formatters-2799225-58.patch23.47 KBjamesdixon
#57 feeds-feeds-item-formatters-2799225-57.patch23.48 KBjamesdixon
#55 interdiff_49-55.txt18.49 KBjamesdixon
#55 feeds-feeds-item-formatters-2799225-55.patch22.44 KBjamesdixon
#54 feeds-feeds-item-formatters-2799225-54.patch20.95 KBjamesdixon
#53 feeds-feeds-item-formatters-2799225-53.patch20.57 KBjamesdixon
#49 interdiff_41-49.txt24.19 KBjamesdixon
#49 feeds-feeds-item-formatters-2799225-49.patch21.68 KBjamesdixon
#48 feeds-feeds-item-formatters-2799225-48.patch20.41 KBjamesdixon
#47 feeds-feeds-item-formatters-2799225-47.patch20.2 KBjamesdixon
#43 feeds-feeds-item-formatters-2799225-43.patch19.46 KBjamesdixon
#43 test-results3.png88.62 KBjamesdixon
#41 test-results2.png113.39 KBjamesdixon
#40 feeds-feeds-item-formatters-2799225-40.patch10.25 KBjamesdixon
#40 test-results.png126.97 KBjamesdixon
#38 phpunit.xml_.txt3.13 KBmegachriz
#37 feeds-feeds-item-formatters-2799225-37.patch10.29 KBjamesdixon
#34 feeds-feeds-item-formatters-2799225-34.patch22.67 KBjamesdixon
#33 mapping.png6.03 KBjamesdixon
#30 feeds-feeds-item-formatters-2799225-29.patch12.54 KBjamesdixon
#29 feeds-feeds-item-formatters-2799225-28.patch11.09 KBjamesdixon
#21 feeds-feeds-item-formatters-2799225-21.patch7.09 KBjcnventura
#14 feeds-feeds-item-hidden-fiddle-do-not-use-2799225-14.patch3.05 KBmegachriz
#5 download.png8.13 KBshobhit_juyal

Comments

saurabh.tripathi.cs created an issue. See original summary.

megachriz’s picture

Status: Active » Closed (won't fix)

The 8.x-2.x version is not supported and heavily out of date. Try the 8.x-3.x version instead.

punamshelke’s picture

Hi,

"Drupal\Component\Plugin\Exception\PluginNotFoundException: The "hidden" plugin does not exist. in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 52 of C:\xampp\htdocs\drupal-current\core\lib\Drupal\Component\Plugin\Discovery\DiscoveryTrait.php)."

I am also getting the same error on drupal 8.3.2 version...

On the content type > faqs > manage fields > feeds Item > edit....
on this page...

you have any idea about this.....

Thanks

vijay.mayilsamy’s picture

It happens to me too on 8.3.x. The problem is with Image field. Need to figure out why the image field is causing that issue.

shobhit_juyal’s picture

StatusFileSize
new8.13 KB

using D 8.5.x and getting the same error for a custom field. Don't know why ?.

nanc2’s picture

I am using D 8.5.3, same error.

Under "manage for display", Feeds Item is disabled and can not enable it.

Inspite of this I still can import feeds with no problems. But I can only map the default fields. I can not map the custom fields.

How can I map feed type fields to custom content type fields?

megachriz’s picture

Priority: Major » Normal
Status: Closed (won't fix) » Active

There is indeed no display formatter for the feeds_item field nor is there a field widget for it. It isn't meant to be manipulated nor displayed: it only contains import metadata.

@nanc2
What do you mean with "custom fields"? Are these fields from a contrib or custom module?

megachriz’s picture

Version: 8.x-2.x-dev » 8.x-3.x-dev
nanc2’s picture

"What do you mean with "custom fields"? Are these fields from a contrib or custom "

The custom fields are the fields added to a feed type and need to be mapped to the content type.

Since the field added to feed type does not show from the mapping source dropdown menu, I tried to add "new source..." and put machine name of the feed type field, but didn't work.

What is the right way to use the "New source..."? How to map the added feed type fields to content type fields? I can only map the predefined fields like title and body.

megachriz’s picture

@nanc2
"New source..." is made for the following purposes:

  1. To define columns used in your CSV file (when importing CSV files).
  2. To define Xpaths when using the XML Xpath parser from Feeds extensible parsers.
  3. To create a "blank" source which you can fill by applying Tamper plugins on it. A common Tamper plugin to apply on such blank source is the "Default value" plugin. That plugin is still in the progress of being ported: #2963016: Plugin: Default Value.

The feature to use feed type fields as mapping sources is being worked on in #2911491: Provide feeds type fields as mapping sources.

nanc2’s picture

Thank you very much for the clarification MegaChriz. I need to import rss feed so I guess I will have to wait.

hongpong’s picture

I also encountered this error on a CSV import related content type field manager. Not a direct problem since we don't need to edit it further in our case.

Notice: Uncaught PHP Exception Drupal\\Component\\Plugin\\Exception\\PluginNotFoundException: "The "hidden" plugin does not exist." at /web/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php line 52, referer: http://SITE/admin/structure/types/manage/CONTENTTYPE/fields

On 8.5.3, feeds 8.x-3.0-alpha1 . I think that this would be best resolved in the short term by making a message appear suggesting people go to the feeds configuration, or something like that.

m.abdulqader’s picture

+1

megachriz’s picture

One way of fixing this is by converting the feeds_item field to a base field in a similar way as the Drupal core module "Path" does. This can be done by implementing hook_entity_base_field_info().

I've been playing around with this (see attached patch), but I'm running into issues by updating existing installs: the previous separate feeds_item field does not get removed and causes PHP errors. Also, the patch adds a feeds_item field to every content entity type, while it should only do that for entity types that you are using in feed types. Plus you need to run drush entity-update to install the field as base field. Ideally, this should happen automatically after saving a feed type.

Do not install the patch on your site: it might break your site.

megachriz’s picture

Status: Active » Needs work
tjtj’s picture

I have this problem. Get error: The node.feeds_item field needs to be uninstalled.
But then, when I try to do it, I get:

# drush entup
The following updates are pending:

node entity type :
The DS switch field needs to be uninstalled.
The node.feeds_item field needs to be uninstalled.

 Do you wish to run all pending updates? (yes/no) [yes]:
 > yes

 [error]  The "field_item:feeds_item" plugin does not exist.
 [error]  The "field_item:feeds_item" plugin does not exist.
 [error]  The "field_item:feeds_item" plugin does not exist.

How do I fix this? I uninstalled feeds, but apparently the uninstall is not clean.
It will not reinstall either:
Unable to install Feeds, system.action.feeds_feed_delete_action already exists in active configuration.

ankitjain28may’s picture

@MegaChriz Hey, Any update on this issue, I am facing the same issue. It would be great if you can give some insights so i will try to fix it.

megachriz’s picture

@ankitjain28may
The approach in #14 could be a good one and may also make uninstalling Feeds easier. There are a few things though that will be hard to get right:

  1. Making sure entity types get a feeds item field when needed.
  2. Updating existing installs.
  3. Removing redundant code as a result of the change.

I think this issue could take a while to get done. It's likely it won't make it in Feeds 8.x-3.0-alpha3, which I hope to release in early September.

ankitjain28may’s picture

Thanks @MegaChriz

jcnventura’s picture

I'm also running into this issue when adding a feed_item field to views. It used to be possible to set a feeds_item field in a view to see the import date, GUID, URL or owner feed nid.

jcnventura’s picture

Status: Needs work » Needs review
StatusFileSize
new7.09 KB

Adding some field formatters to allow the feeds_item field usage in Views. This should also get rid of the PluginNotFoundException.

No interdiff as the BaseFieldDefinition is it's own thing. I'm creating a separate issue for that one.

vijay.mayilsamy’s picture

Applied the patch. Editing the feed item works fine.. Not getting that error.

It will be very useful if we develop a test case for this one ?

Thanks

megachriz’s picture

Issue tags: +Needs tests

@vijay.mayilsamy
Yes, tests would be useful for this one. I think we need a functional test for each formatter. A simple example may be the test for the telephone_link formatter in /core/modules/telephone/tests/src/Functional/TelephoneFieldTest.php:

/**
 * Test the telephone formatter.
 *
 * @covers \Drupal\telephone\Plugin\Field\FieldFormatter\TelephoneLinkFormatter::viewElements
 *
 * @dataProvider providerPhoneNumbers
 */
public function testTelephoneFormatter($input, $expected) {
  // Test basic entry of telephone field.
  $edit = [
    'title[0][value]' => $this->randomMachineName(),
    'field_telephone[0][value]' => $input,
  ];

  $this->drupalPostForm('node/add/article', $edit, t('Save'));
  $this->assertRaw('<a href="tel:' . $expected . '">');
}

/**
 * Provides the phone numbers to check and expected results.
 */
public function providerPhoneNumbers() {
  return [
    'standard phone number' => ['123456789', '123456789'],
    'whitespace is removed' => ['1234 56789', '123456789'],
    'parse_url(0) return FALSE workaround' => ['0', '0-'],
    'php bug 70588 workaround - lower edge check' => ['1', '1-'],
    'php bug 70588 workaround' => ['123', '1-23'],
    'php bug 70588 workaround - with whitespace removal' => ['1 2 3 4 5', '1-2345'],
    'php bug 70588 workaround - upper edge check' => ['65534', '6-5534'],
    'php bug 70588 workaround - edge check' => ['65535', '6-5535'],
    'php bug 70588 workaround - invalid port number - lower edge check' => ['65536', '6-5536'],
    'php bug 70588 workaround - invalid port number - upper edge check' => ['99999', '9-9999'],
    'lowest number not affected by php bug 70588' => ['100000', '100000'],
  ];
}

Only difference is that the first few lines of ::testTelephoneFormatter() cannot be used, since there is no widget for the feeds_item field. So instead the feeds_item field need to be filled programmatically. Something like this:

$entity->feeds_item->guid = $guid;
$entity->save();
$this->drupalGet($entity->url());
vijay.mayilsamy’s picture

Status: Needs review » Needs work
vijay.mayilsamy’s picture

Hi MegaChriz,

a) So, What you are saying is we need to have a test case for the following FieldFormatters ?

  • ::testGUIDFormatter() feeds_item_guid (FeedsItemGuidFormatter.php)
  • ::testImportedFormatter() feeds_item_imported (FeedsItemImportedFormatter.php)
  • ::testTargetFormatter() feeds_item_target (FeedsItemTargetFormatter.php)
  • ::testUrlFormatter() feeds_item_url (FeedsItemUrlFormatter.php)

b) BrowserTestBase should be extended in the test like ::testTelephoneFormatter() ?

c) On the tests we add the test case to check the fields after formatting the text ?

Thanks
Vijay

megachriz’s picture

@vijay.mayilsamy
a) Yes, we need have a test for every field formatter added by the patch in #21. testGUIDFormatter() should be called testGuidFormatter(). The Drupal coding standards say that you should not uppercase abbreviations in method, class or variable names.
b) Extend \Drupal\Tests\feeds\Functional\FeedsBrowserTestBase. This will give you the article node type and helper methods, like a method for creating a field: ::createFieldWithStorage() in FeedsCommonTrait.php.
c) Yes, the output of the formatter should be checked. It is important that if a formatter has several ways of outputting something, each way should be at least covered once. For example, the FeedsItemGuidFormatter has three ways of outputting the value: as nothing (value is empty), as an url or as plain text.

Do you want to give implementing the tests a try?

jamesdixon’s picture

Assigned: Unassigned » jamesdixon

Thanks for the direction @MegaChriz I'll take these tests on this week.

jamesdixon’s picture

Had a chance to explore other functional feeds tests. For my own reference I believe this test is a good example along with some others in the same directory:

feeds/tests/src/Functional/FieldValidationTest.php

That and also what @MegaChriz sugested from core:

/core/modules/telephone/tests/src/Functional/TelephoneFieldTest.php
jamesdixon’s picture

Uploading a very rough placeholder first test which needs implementing. Good skeleton to work with later.

jamesdixon’s picture

Adding small progress here.

Right now I'm a bit confused about where these field formatters are outputting what they are returning from viewElement().

When I go to view a feed GUID is not shown for example. We cannot view GUID from a feed type because there is no view for feed type, at least I couldn't figure out how to do this.

So when submitting either form I am not sure where to check for the GUID value in the browser, as the telephone field formatter has for example.

megachriz’s picture

I think the tests should follow these steps:

  1. Start with an empty test class extending FeedsBrowserTestBase.
  2. Create the feeds_item field (call ::createFieldWithStorage(), which by default adds a field to the article content type).
  3. Programmatically configure display settings for this field (select formatter, set settings).
  4. Programmatically create an article node, with a value on the feeds_item field.
  5. Display the article node.
  6. Assert that the displayed value is the expected value.
jamesdixon’s picture

Oh man that's where the disconnect was happening. For some reason I thought the field formatters were being applied when viewing a feed or feed type. It is actually being displayed on a node created by a feed. Thanks for the direction @MegaChriz, that makes good sense!

jamesdixon’s picture

StatusFileSize
new6.03 KB

Found a good way to test what shows from frontend. As @MegaChriz mentioned you have to have a Feed Type which imports to the entity for that entity to get a feeds_item field.

Noting that to get Item URL and Item GUID to actually show anything when setting that field formatter you have to map to them like in the screenshot for example.

jamesdixon’s picture

Made a bit of progress on the FeedsItemGuidFormatter but unable to test my code because of some dependency issue with PHPUnit and my environment:

Fatal error: require(): Failed opening required '/app/vendor/composer/../phpunit/phpunit/src/ForwardCompatibility/TestCase.php' (include_path='.:/usr/local/lib/php') in /app/vendor/symfony/class-loader/ApcClassLoader.php on line 112 

Will see if I can sort this out tomorrow.

megachriz’s picture

@jamesdixon

  1. +++ b/tests/src/Functional/FeedsItemGuidFormatterTest.php
    @@ -0,0 +1,105 @@
    +    $this->createFieldWithStorage('feeds_item');
    

    This creates a field of type 'text' (as that is what ::createFieldWithStorage() chooses as the default field type). You'll need to create a field of type 'feeds_item'.

  2. +++ b/tests/src/Functional/FeedsItemGuidFormatterTest.php
    @@ -0,0 +1,105 @@
    +    $format = FilterFormat::create([
    +      'format' => 'feeds_item_guid',
    +      'name' => 'GUID of feed item',
    +    ]);
    +    $format->save();
    

    We don't need to set filters here. I know these are created in other tests of Feeds, but in these tests specifically formatted text fields are tested (like the body field that Drupal adds by default to content types, for example).
    We don't test text fields here, but feed_item fields.

    It are the display settings of the article content type that need to be set here instead.

  3. While the output of a feeds_item field could technically be tested by performing an import first, I think it will be much simpler to set it programmatically:
    $entity->feeds_item->guid = $guid;
    $entity->save();

    And then display the node:
    $this->drupalGet($entity->url());

    The pro's of this approach are that the test code becomes simpler and that you test the feeds_item field type in isolation.
    The con would be that if the import process would change in a way that the the feeds_item properties would change from data type (weird example: if at some point it is decided that feeds_item->guid will hold an array value instead of a string), then this approach will not cover that change.

About the fatal error: did you move classes? Maybe it helps to remove phpunit from the vendor folder and perform a composer install again?

jamesdixon’s picture

Thanks for the direction @MegaChriz!

Right I missed it was setting type to text by default, I'll switch that to feeds_item.

Sounds good I'll configure the display settings of the article instead of creating a formatter.

I like the idea of skipping the import it will make these tests simpler.

Good call on wiping the vendor folder and composer installing again I will try that.

jamesdixon’s picture

It's been challenging, mostly because this is my first functional test in D8. Making progress though!

Attached is an example of what I expect these tests might look like based on guidance from @MegaChriz. I haven't tested this yet though.

Still having troubles with my dev environment. Nuking vendor and rebuilding with composer worked well, but I am unable to run any functional tests at all because Lando is having trouble accessing itself while running functional tests:

1) Drupal\Tests\telephone\Functional\TelephoneFieldTest::testTelephoneWidget
GuzzleHttp\Exception\ConnectException: cURL error 7: Failed to connect to drupal.lndo.site port 8000: Connection refused (see http://curl.haxx.se/libcurl/c/libcurl-errors.html)

Looks like I need to add some tooling to Lando or mess with my firewall rules. Still working on this.

megachriz’s picture

StatusFileSize
new3.13 KB

@jamesdixon
It looks like the patch is heading in the right direction! I'm not sure yet if it is needed to create a feed type and feed in the test specifically for the 'guid' property, but maybe it is a good idea to leave it in to have a valid value for the' target_id' property.

I don't know anything about Lando, but I execute tests this way:
./vendor/bin/phpunit -v modules/wip/feeds8/feeds/tests/src/Functional/Feeds/Target/FileTest.php
Attached the phpunit.xml file that I have placed in the drupal root folder.

jamesdixon’s picture

This may not belong here, but writing it down so I don't forget. In case anyone else runs into issues running functional tests with Lando here's how I got around it. Tests keep timing out from the testing user interface in Drupal admin and I couldn't fix that issue after trying on multiple machines:

1) Drupal\Tests\telephone\Functional\TelephoneFieldTest::testTelephoneWidget
GuzzleHttp\Exception\ConnectException: cURL error 7: Failed to connect to drupal8phpunit.lndo.site port 8000: Connection refused (see http://curl.haxx.se/libcurl/c/libcurl-errors.html)

I found a recipe online which lets you run phpunit tests from the command line with Drupal8:

https://github.com/finnef/lando-drupal8-test-debugging

1) Grab the recipe with git and lando start from the lando recipe directory. It will setup the virtual server install drupal etc.
2) Run your test commands like this from the lando recipe directory:
lando phpunit "/app/web/core/modules/telephone/tests/src/Functional/TelephoneFieldTest.php"

Yay I can finally test this sucker out!

jamesdixon’s picture

StatusFileSize
new126.97 KB
new10.25 KB

Success! The FeedsItemGUIDFormatter test is complete and passes:

3 test left to go, I'll work through those for this Thursday.

jamesdixon’s picture

StatusFileSize
new13.43 KB
new113.39 KB

FeedsItemUrlFormatter test is complete and passes:

I'm noticing the option to select

modules/feeds/src/Plugin/Field/FieldFormatter/FeedsUriLinkFormatter.php

from the feeds_item display options is not appearing. I wonder if there is some issue with that field formatter or if there is something wrong with my development environment. Will revisit this when the other tests are completed.

andypost’s picture

  1. +++ b/src/Plugin/Field/FieldFormatter/FeedsItemGuidFormatter.php
    @@ -0,0 +1,55 @@
    +      if ($item->isEmpty()) {
    +        continue;
    +      }
    ...
    +      if (!empty($item->get('guid'))) {
    +        $guid = $item->get('guid')->getValue();
    

    I bet $guid = $item->guid; should be enough and there's also check for empty item

  2. +++ b/src/Plugin/Field/FieldFormatter/FeedsItemGuidFormatter.php
    @@ -0,0 +1,55 @@
    +        $scheme = parse_url($guid, PHP_URL_SCHEME);
    +        if ($scheme === 'http' || $scheme === 'https') {
    +          $elements[$delta] = [
    
    +++ b/src/Plugin/Field/FieldFormatter/FeedsItemUrlFormatter.php
    @@ -0,0 +1,55 @@
    +        $scheme = parse_url($uri, PHP_URL_SCHEME);
    +        if ($scheme === 'http' || $scheme === 'https') {
    +          $elements[$delta] = [
    +            '#type' => 'link',
    

    code duplication better to move to base class

  3. +++ b/src/Plugin/Field/FieldFormatter/FeedsItemUrlFormatter.php
    @@ -0,0 +1,55 @@
    +        $uri = $item->get('url')->getValue();
    

    do not use getValue() it returns raw data, field items has properties

jamesdixon’s picture

StatusFileSize
new88.62 KB
new19.46 KB

Thanks for the review @andypost.

For 1. Do you mean just access $item->guid instead of $item->get('guid')?

For 2. I'm a fan of DRY programming, I'm wondering what I'd call the base class to set up that scheme stuff.

For 3. Are you suggesting we use $item->url instead of $item->get('url')

Outside of code review, we're on the home stretch! There are now 4 tests. I'm just figuring out how to properly set the date format, custom date format, and timezone when displaying the imported field formatter. Now I am getting:

1) Drupal\Tests\feeds\Functional\FeedsItemImportedFormatterTest::testFeedsItemImportedFormatter with data set "empty imported" ('', '<div class="field__item"></div>')
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for core.entity_view_display.node.article.default with the following errors: core.entity_view_display.node.article.default:content.feeds_item.settings.date_format missing schema, core.entity_view_display.node.article.default:content.feeds_item.settings.custom_date_format missing schema, core.entity_view_display.node.article.default:content.feeds_item.settings.timezone missing schema

FeedsItemTargetFormatterTest is passing strong though!

megachriz’s picture

  1. I would indeed use $item->guid for accessing the guid value. I'm actually not sure what object $item->get('guid') gives. I should check that.
  2. I think FeedsItemFormatterBase could be a good name. All formatters for feeds_item can then extend this class. You could add a separate method for handling the url stuff and only the formatters that need that would call that method. About naming that method: perhaps two methods would work best here: isUrl() (or looksLikeUrl() to not confuse it with that it would check for an Url object?) for checking if the value looks like an url and viewUrl() for generating the render array.
  3. Similar as point 1, I would use $item->url.

Schema errors

The error message "Schema errors for ... missing schema" means that configuration keys are expected to be defined in feeds.schema.yml, but they aren't. You need to do something similar as what you did for several Tamper plugins here. The formatter FeedsItemImportedFormatter has configuration (inherited from TimestampFormatter). Since the formatter doesn't declare configuration on its own, you would need in the config schema define that the config for this formatter inherits from TimestampFormatter.
I think that would use a pattern like the following:

action.configuration.feeds_feed_delete_action:
  type: action_configuration_default
  label: 'Delete feeds configuration'

The first line would be the config key for the feeds item formatter. The type would be the config key from the formatter which config is being inherited, thus the config key from TimestampFormatter.

megachriz’s picture

Forgot one thing: a suggestion about where to place the test classes. If a test class focusses on covering a class in particular, I would use the same folder structure for the test class as for the class being tested. I think that this way the test classes get better organized.
An example: FeedsItemGuidFormatter is in /src/Plugin/Field/FieldFormatter. I would then place FeedsItemGuidFormatterTest in /tests/src/Functional/Plugin/Field/FieldFormatter (and update the namespace accordingly).

jamesdixon’s picture

Good points there and all will help me wrap this up. Thanks @MegaChriz for the guidance.

jamesdixon’s picture

Figured out the schema we need. Some work still needed on the tests, but small progress.

jamesdixon’s picture

Got the final tests passing. Now we just need to refactor the code a little based on feedback.

jamesdixon’s picture

Status: Needs work » Needs review
StatusFileSize
new21.68 KB
new24.19 KB

Tests are passing and I've:

1) Stopped using that get function and directly access the property values from the field formatters
2) Moved tests into it's own directory
3) Created a new base class for feeds item field formatters to stop code duplication in a couple field formatters

Included is an interdiff since the last code review.

jamesdixon’s picture

Assigned: jamesdixon » Unassigned
megachriz’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

It took me some time to go through all the code (about 1.5 hours I think?), but here is my review. Great work by the way!

  1. Case sensitive issue

    +++ b/src/Plugin/Field/FieldFormatter/FeedsItemGuidFormatter.php
    @@ -0,0 +1,27 @@
    +    return $this->viewUrlOrHTMLElement('guid', $items);
    
    +++ b/src/Plugin/Field/FieldFormatter/FeedsItemUrlFormatter.php
    @@ -0,0 +1,27 @@
    +    return $this->viewUrlOrHTMLElement('url', $items);
    

    "HTML" is in uppercase here, while in the method actual method it is "Html". This causes issues on case sensitive systems.

  2. @group feeds

    +++ b/src/Plugin/Field/FieldFormatter/FeedsItemFormatterBase.php
    @@ -0,0 +1,54 @@
    + * @group feeds
    

    Annotation for @group is only needed for tests.

  3. Code duplication in tests

    There is some code duplication in the tests. I think it is a good idea to create a base class formatter tests called "FeedsItemFormatterTestBase" that would just contain this:

      /**
       * {@inheritdoc}
       */
      public static $modules = [
        'feeds',
        'node',
        'user',
        'file',
        'field',
      ];
    
      /**
       * {@inheritdoc}
       */
      protected function setUp() {
        parent::setUp();
    
        // Create feeds_item field.
        $config = [
          'type' => 'feeds_item',
        ];
        $this->createFieldWithStorage('feeds_item', $config);
      }
    
    
  4. Test code organisation

    +++ b/tests/src/Functional/Plugin/Field/FieldFormatter/FeedsItemGuidFormatterTest.php
    @@ -0,0 +1,100 @@
    +    // Create a feed type, map to feeds_item fields guid.
    +    $feed_type = $this->createFeedTypeForCsv(['guid', 'title'], [
    +      'mappings' => array_merge($this->getDefaultMappings(), [
    +        [
    +          'target' => 'feeds_item',
    +          'map' => ['guid' => 'guid'],
    +        ],
    +      ]),
    +    ]);
    +
    +    // Create a feed for the article to belong to.
    +    $feed = $this->createFeed($feed_type->id(), [
    +      'source' => $this->resourcesPath() . '/csv/content.csv',
    +    ]);
    

    To reduce code duplication, this code could be added to the base test class "FeedsItemFormatterTestBase" (mentioned earlier) in a separate method. In fact, the mappings do not really matter as the test doesn't perform an actual import. A value is set directly on $article->feeds_item.

  5. Split up viewUrlOrHtmlElement()?

    +++ b/src/Plugin/Field/FieldFormatter/FeedsItemFormatterBase.php
    @@ -0,0 +1,54 @@
    +  /**
    +   * View Url or HTML element.
    +   *
    +   * @param string $field_id
    +   *   Id of the feeds item field you are displaying (ie: guid).
    +   * @param \Drupal\Core\Field\FieldItemListInterface $items
    +   *   Feeds items that are being displayed.
    +   */
    +  public function viewUrlOrHtmlElement($field_id, FieldItemListInterface $items) {
    +    $elements = [];
    +    foreach ($items as $delta => $item) {
    +      if ($item->isEmpty()) {
    +        continue;
    +      }
    +
    +      if (!empty($item->{$field_id})) {
    +        $field_value = $item->{$field_id};
    +
    +        $scheme = parse_url($field_value, PHP_URL_SCHEME);
    +        if ($scheme === 'http' || $scheme === 'https') {
    +          $elements[$delta] = [
    +            '#type' => 'link',
    +            '#url' => Url::fromUri($field_value),
    +            '#title' => $field_value,
    +          ];
    +        }
    +        else {
    +          $elements[$delta] = [
    +            '#markup' => Html::escape($field_value),
    +          ];
    +        }
    +      }
    +    }
    +
    +    return $elements;
    +  }
    

    For avoiding code duplication, this is great. However, the complexity of the code could be reduced. I think it would be good idea to split this up into separate methods: one that checks if the value is an URL, one that generates the URL and one that generates plain text.

    This has something to do with my thoughts about the url formatter, see further.

  6. Imported formatter

    +++ b/src/Plugin/Field/FieldFormatter/FeedsItemImportedFormatter.php
    @@ -0,0 +1,58 @@
    +          '#markup' => $this->dateFormatter->format($item->get('imported')->getValue(), $date_format, $custom_date_format, $timezone, $langcode),
    

    $item->get('imported')->getValue() can be changed to $item->imported. I don't know what exactly the differences are between the two formats, but at least $item->imported is shorter.

  7. Target formatter (1)

    +++ b/src/Plugin/Field/FieldFormatter/FeedsItemTargetFormatter.php
    @@ -0,0 +1,42 @@
    +          '#markup' => Html::escape($item->get('target_id')->getValue()),
    

    Same. $item->get('target_id')->getValue() can be changed to $item->target_id.

  8. Target formatter (2)

    +++ b/src/Plugin/Field/FieldFormatter/FeedsItemTargetFormatter.php
    @@ -0,0 +1,42 @@
    + *   label = @Translation("Node id of the owner feed node"),
    

    This label is somewhat misleading. A feed entity is not a node. "Feed ID" would be correct here.

  9. Target formatter (3)

    +++ b/src/Plugin/Field/FieldFormatter/FeedsItemTargetFormatter.php
    @@ -0,0 +1,42 @@
    +      if (!empty($item->get('target_id'))) {
    +        $elements[$delta] = [
    +          '#markup' => Html::escape($item->get('target_id')->getValue()),
    +        ];
    +      }
    
    +++ b/tests/src/Functional/Plugin/Field/FieldFormatter/FeedsItemTargetFormatterTest.php
    @@ -0,0 +1,92 @@
    +      'html with target id' => ['<em>Skeletor!!!!</em>', '<div class="field__item">&lt;em&gt;Skeletor!!!!&lt;/em&gt;</div>'],
    

    This doesn't look right. Target ID can never be html. It would be more useful if this formatter would give you options for displaying the feed's label, ID or rendered entity. That could be a lot of work to add, so for this issue it would be good enough if it was just be able to display the feed ID. So in that case the formatter should no try to escape html, but just convert the value to an integer.

  10. Url formatter

    +++ b/src/Plugin/Field/FieldFormatter/FeedsItemUrlFormatter.php
    @@ -0,0 +1,27 @@
    +    return $this->viewUrlOrHTMLElement('url', $items);
    
    +++ b/tests/src/Functional/Plugin/Field/FieldFormatter/FeedsItemUrlFormatterTest.php
    @@ -0,0 +1,102 @@
    +      'non http or https html url' => ['<strong>SkyNet activated</strong>', '<div class="field__item">&lt;strong&gt;SkyNet activated&lt;/strong&gt;</div>'],
    

    I'm not sure if we should expect the URL to be plain text. I think we should expect that this property of the feeds_item field should either be an url or be empty. Now I come back at an earlier point: why it is a good idea to split up viewUrlOrHtmlElement(). The url property should always be expected to contain an url, not HTML. In case the property contains an invalid url, it should display nothing, I think? We should check how the core link module handles this.

    I think it would also be useful to add a configuration option to render the url as plain text, just as for link fields. This way, in Views you can use the plain url result to turn an other field into an url.

jamesdixon’s picture

Assigned: Unassigned » jamesdixon

Very thorough review thank you!

Yes felt like a bit of code duplication in the tests. Will be good to create a Base Test class.

Some of those tests with HTML in URLs etc didn't seem right. The only reason I did it was because the URL values were being escaped for HTML. I like the suggestions there.

I plan on wrapping this one up this week.

jamesdixon’s picture

Here's some progress. Things that remain are testing it out and point 10 from @MegaChriz's last review.

jamesdixon’s picture

There were some errors in my last patch.

This one fixes those.

I am still working on configuration from @MegaChriz's last suggestion number 10.

jamesdixon’s picture

StatusFileSize
new22.44 KB
new18.49 KB

I'm 95% of the way there but am having a hard time figuring out how to programatically set a setting for field formatters in a test FeedsItemUrlFormatterTest.php.

When the url value is https://en.wikipedia.org/wiki/Star_Control_3 my plan is to set that plain text url setting for the formatter to TRUE and make sure it is not turned into an <a> tag. Should I be submitting the form for that somehow beforehand or manually setting that configuration value somehow?

megachriz’s picture

@jamesdixon
For testing the url to be outputted as plain text, I would create a new test method called something like testOutputUrlAsPlainText. In there, change the settings of the formatter first by passing settings to setComponent(), just as you did in FeedsItemImportedFormatterTest.

Then something else. I see that an empty url results into '<div class="field__item"></div>'. I wonder if it should instead result into ''? It would be good to check what the formatter for the core link field does when you give it an empty value. Maybe Drupal skips executing the formatter if it is given an empty value?

jamesdixon’s picture

Thanks for the direction, I'll look into the empty results best practices.

Here's the new plain text test but it's not displaying the url properly for some reason. Will debug later on.

jamesdixon’s picture

Status: Needs work » Needs review
StatusFileSize
new23.47 KB
new4.91 KB

Sorry for the delay here. I had more work over the holidays than I expected.

I have the plain text URL feature and test working now.

I looked into the LinkFieldTest.php: https://api.drupal.org/api/drupal/core%21modules%21link%21tests%21src%21...

The testLinkFormatter() function doesn't even appear to be testing for an empty URL input.

Maybe we should drop the check for the empty URL?

megachriz’s picture

Title: The "hidden" plugin does not exist: Drupal\Component\Plugin\Exception\PluginNotFoundException: » The "hidden" plugin does not exist: Drupal\Component\Plugin\Exception\PluginNotFoundException:
StatusFileSize
new25.07 KB
new10.62 KB

@jamesdixon
I have fiddled a bit with the url formatter to get a better idea of what to do with these empty values. It appears to have resulted into a new patch with quite some changes. It could be that the tests need to be updated.

I looked especially at what LinkFormatter did and I made the following changes accordingly:

  • The link field is passed to #plain_text if configured to show as plain text. So I did the same in FeedsItemUrlFormatter::generateLink(). Also did this for FeedsItemGuidFormatter.
  • To generate the url, there is a specific buildUrl() method for the link field formatter, so I added a method with the same name to FeedsItemUrlFormatter.
  • The setting for showing the url as plain text, is called "url_plain", so I changed the name of that setting as well for FeedsItemUrlFormatter.
  • The default value for "url_plain" should be empty, according to the link field formatter.
  • The config form element for "url_plain" should be "checkbox" instead of "boolean".
  • FeedsItemUrlFormatter::settingsSummary() was missing, so I added that.
  • If formatting $feeds_item->url as url fails, nothing should be displayed. When it fails, an exception is thrown by \Drupal\Core\Url so that one is catched.
  • A method called getUrl() is added to FeedsItem, together with an accompinied interface called FeedsItemInterface.

To further improve the url formatter, we could add more options from the link field formatter (trim_length, rel, target). But then these need tests as well.

I have some ideas about improving the FeedsItemTargetFormatter as well. This will follow later today.

Status: Needs review » Needs work

The last submitted patch, 59: feeds-feeds-item-formatters-2799225-59.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

megachriz’s picture

Status: Needs work » Needs review
StatusFileSize
new24.93 KB
new3.62 KB

I returned the wrong thing in FeedsItem::getUrl() and FeedsItemGuidFormatter missed an import. I also changed the FeedsItemUrlFormatterTest a bit. I first thought that ::providerUrls() expected that <div class="field__item"></div> was displayed for an empty value, but by further expecting the code, it appears that the opposite was expected. So I hope I made that more clear.

Let's see if this works better.

Status: Needs review » Needs work

The last submitted patch, 61: feeds-feeds-item-formatters-2799225-61.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

megachriz’s picture

Status: Needs work » Needs review
StatusFileSize
new25.08 KB
new5.38 KB
  • <strong>SkyNet activated</strong> should result into nothing being displayed for the url formatter.
  • By rendering the guid as url, I made a mistake. I wrote $this->guid, should have been $item->guid.
  • I changed the data providers for the url formatter and the guid formatter to pass NULL instead when it is expected that nothing gets rendered. Accordingly, testFeedsItemUrlFormatter() and testFeedsItemGuidFormatter() check if the feeds_item field is not displayed at all.

Status: Needs review » Needs work

The last submitted patch, 63: feeds-feeds-item-formatters-2799225-63.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

megachriz’s picture

Status: Needs work » Needs review
StatusFileSize
new25.13 KB
new1.71 KB

I think that a guid of 0 should be displayed. Fixed the issue of displaying the field when guid is empty.

megachriz’s picture

Finally, it was about time tests get passing again! Now onto changes for the target_id formatter.

This adds additional formatters for $feeds_item->target_id. They all extend existing formatters for entityreference fields.

  • FeedsItemTargetEntityFormatter to display a "rendered feed".
  • FeedsItemTargetIdFormatter to display the feed ID.
  • FeedsItemTargetLabelFormatter to display the feed title.

Other changes:

  • Added the method assertFeedsItemFieldNotDisplayed() for checking if the feeds_item field is not displayed. The tests for the formatters for guid, url and target_id call this method.
  • The field type FeedsItem now uses the list class \Drupal\Core\Field\EntityReferenceFieldItemList. This is needed because EntityReferenceFormatterBase::getEntitiesToView() expect the list class to be an instance of \Drupal\Core\Field\EntityReferenceFieldItemListInterface and the default field list class doesn't implement that interface.
  • To FeedsItemTargetIdFormatterTest a case is added for referencing a non-existing feed ID. Not tested, but I think in that case the ID should not be displayed.
  • The implementation for FeedsItemTargetIdFormatter is completely emptied, all logic already happens in EntityReferenceIdFormatter.

Let's see if these changes still get the tests on green.

jamesdixon’s picture

Status: Needs review » Needs work

Some great work @MegaChriz!

I love the code reuse for the target formatters.

Also a huge upgrade to the Url formatter, it works much closer to core.

The way you're checking $expected for NULL and reusing the function to ensure nothing was rendered is some awesome DRY code.

We need to create the following tests:

feeds/tests/src/Functional/Plugin/Field/FieldFormatter/FeedsItemTargetEntityFormatterTest.php
feeds/tests/src/Functional/Plugin/Field/FieldFormatter/FeedsItemTargetLabelFormatterTest.php

Which test:

feeds/src/Plugin/Field/FieldFormatter/FeedsItemTargetEntityFormatter.php
feeds/src/Plugin/Field/FieldFormatter/FeedsItemTargetLabelFormatter.php

I haven't had as much time as I hoped for this in the week: January deadlines are looming. Hope to knock these off shortly. They shouldn't be hard we can duplicate FeedsItemTargetIdFormatterTest.php and tweak them for the expected results from the new formatters.

jamesdixon’s picture

FeedsItemTargetEntityFormatterTest.php is actually a bit more involved because we're rendering the feeds item associated with the article.

Made a start on it. I think we'll need to set some display configuration for the feed item itself and also set expected values for things like source, items imported, last imported and next import.

FeedsItemTargetLabelFormatterTest.php is just a placeholder for now copied from FeedsItemTargetIdFormatterTest.php, so running tests will likely fail with this patch.

It's a start on these final two tests though!

jamesdixon’s picture

Some direction from @megachriz in slack:

1) Remove quickedit

2) Drupal\Tests\field\Kernel\EntityReference\EntityReferenceFormatterTest

/**
   * Tests the entity formatter.
   */
  public function testEntityFormatter() {
    /** @var \Drupal\Core\Render\RendererInterface $renderer */
    $renderer = $this->container->get('renderer');
    $formatter = 'entity_reference_entity_view';
    $build = $this->buildRenderArray([$this->referencedEntity, $this->unsavedReferencedEntity], $formatter);

    // Test the first field item.
    $expected_rendered_name_field_1 = '
            <div class="field field--name-name field--type-string field--label-hidden field__item">' . $this->referencedEntity->label() . '</div>
      ';
    $expected_rendered_body_field_1 = '
  <div class="clearfix text-formatted field field--name-body field--type-text field--label-above">
    <div class="field__label">Body</div>
              <div class="field__item"><p>Hello, world!</p></div>
          </div>
';
    $renderer->renderRoot($build[0]);
    $this->assertEqual($build[0]['#markup'], 'default | ' . $this->referencedEntity->label() . $expected_rendered_name_field_1 . $expected_rendered_body_field_1, sprintf('The markup returned by the %s formatter is correct for an item with a saved entity.', $formatter));
    $expected_cache_tags = Cache::mergeTags(\Drupal::entityManager()->getViewBuilder($this->entityType)->getCacheTags(), $this->referencedEntity->getCacheTags());
    $expected_cache_tags = Cache::mergeTags($expected_cache_tags, FilterFormat::load('full_html')->getCacheTags());
    $this->assertEqual($build[0]['#cache']['tags'], $expected_cache_tags, format_string('The @formatter formatter has the expected cache tags.', ['@formatter' => $formatter]));

    // Test the second field item.
    $expected_rendered_name_field_2 = '
            <div class="field field--name-name field--type-string field--label-hidden field__item">' . $this->unsavedReferencedEntity->label() . '</div>
      ';
    $expected_rendered_body_field_2 = '
  <div class="clearfix text-formatted field field--name-body field--type-text field--label-above">
    <div class="field__label">Body</div>
              <div class="field__item"><p>Hello, unsaved world!</p></div>
          </div>
';

    $renderer->renderRoot($build[1]);
    $this->assertEqual($build[1]['#markup'], 'default | ' . $this->unsavedReferencedEntity->label() . $expected_rendered_name_field_2 . $expected_rendered_body_field_2, sprintf('The markup returned by the %s formatter is correct for an item with a unsaved entity.', $formatter));
  }

So we could also just add a field to the feed entity in the test and check if that is displayed.

paulgranted’s picture

Noob here - I can't apply this patch to the feeds folder.
Currently using 8.5.0 as part of an upgrade path and I have the following error:
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "field_item:feeds_item" plugin does not exist. in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 52 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).
Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition(Array, 'field_item:feeds_item', 1) (Line: 25)
Drupal\Core\Plugin\DefaultPluginManager->getDefinition('field_item:feeds_item') (Line: 795)
Drupal\field\Entity\FieldStorageConfig->getFieldItemClass() (Line: 447)
Drupal\field\Entity\FieldStorageConfig->getSchema() (Line: 485)
Drupal\field\Entity\FieldStorageConfig->getColumns() (Line: 1684)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->countFieldData(Object, 1) (Line: 447)
Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->onFieldStorageDefinitionDelete(Object) (Line: 1511)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->Drupal\Core\Entity\Sql\{closure}() (Line: 1527)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->wrapSchemaException(Object) (Line: 1512)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->onFieldStorageDefinitionDelete(Object) (Line: 172)
Drupal\Core\Field\FieldStorageDefinitionListener->onFieldStorageDefinitionDelete(Object) (Line: 599)
Drupal\Core\Entity\EntityManager->onFieldStorageDefinitionDelete(Object) (Line: 242)
Drupal\Core\Entity\EntityDefinitionUpdateManager->doFieldUpdate(3, NULL, Object) (Line: 116)
Drupal\Core\Entity\EntityDefinitionUpdateManager->applyUpdates() (Line: 24)

I have installed drupal/feeds using composer:
sudo composer require 'drupal/feeds:^3.0'

Any help greatly appreciated.

**update - applied the patch : feeds_remove_deleted_field_mappings-2969617-10.patch
from:
https://www.drupal.org/project/feeds/issues/2969617#comment-12634392
Now able to edit feed types that were throwing errors when going to mappings.

Running drush updb: FAILS
[error] Update aborted by: feeds_update_8001
[error] Finished performing updates.

jamesdixon’s picture

@paulgranted: Apologies, the more recent patches are a work in progress and not ready for review yet.

I haven't tested it out yet, but here's a proposed test for the FeedsItemTargetEntityFormatterTest.

Will test it out and finish the FeedsItemTargetLabelFormatterTest soon.

jamesdixon’s picture

megachriz [10:22 AM]
@jamesdixon I made a method for adding a field in one of the Feeds traits.
That could decrease the amount of the code in the test a bit.

jamesdixon [10:23 AM]
Oh cool
Thanks for the tip

megachriz [10:24 AM]
You do need to read the method and its contents carefully at first.

But is saves at least a class import.

jamesdixon’s picture

I've included @MegaChriz's suggestions. I'm getting an error I'm confused about testing the latest version of the FeedsItemTargetEntityFormatterTest. It's asking me to include schema for the article's feeds_item settings view_mode, which it hasn't asked for in the past when setting up this kind of test.

1) Drupal\Tests\feeds\Functional\Plugin\Field\FieldFormatter\FeedsItemTargetEntityFormatterTest::testFeedsItemTargetEntityFormatter
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for core.entity_view_display.node.article.default with the following errors: core.entity_view_display.node.article.default:content.feeds_item.settings.view_mode missing schema, core.entity_view_display.node.article.default:content.feeds_item.settings.link missing schema
jamesdixon’s picture

Sorry had a swp file in the last patch, fixed that now.

jamesdixon’s picture

For my own reference this is what the output looks like for the label formatter:

With link setting on:

<div data-quickedit-field-id="node/130/feeds_item/en/full" class="field field--name-feeds-item field--type-feeds-item field--label-above quickedit-field">
    <div class="field__label">Feeds item</div>
              <div class="field__item"><a href="/feed/1" hreflang="und">Video Games</a></div>
          </div>

With link setting off:

<div data-quickedit-field-id="node/130/feeds_item/en/full" class="field field--name-feeds-item field--type-feeds-item field--label-above quickedit-field">
    <div class="field__label">Feeds item</div>
              <div class="field__item">Video Games</div>
          </div>

Will need to strip quickedit info out of there too.

jamesdixon’s picture

Created a test for FeedsItemTargetLabelFormatterTest but it's broken too because of the same type of error the FeedsItemTargetEntityFormatterTest is getting in #73.

It looks like it wants us to create schema for the article view mode feeds_item settings. Not sure how to get around this, or whether we actually need to. This has something to do with us using core formatters.

1) Drupal\Tests\feeds\Functional\Plugin\Field\FieldFormatter\FeedsItemTargetLabelFormatterTest::testFeedsItemTargetLabelFormatterPlain
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for core.entity_view_display.node.article.default with the following errors: core.entity_view_display.node.article.default:content.feeds_item.settings.link missing schema

/app/web/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:95
/app/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:111
/app/web/core/lib/Drupal/Core/Config/Config.php:231
/app/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:284
/app/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:429
/app/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:263
/app/web/core/lib/Drupal/Core/Entity/Entity.php:390
/app/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:632
/app/web/modules/feeds/tests/src/Functional/Plugin/Field/FieldFormatter/FeedsItemTargetLabelFormatterTest.php:29
jamesdixon’s picture

Forgot to attach files.

megachriz’s picture

@jamesdixon
The field formatters that extend the entity reference field formatters are the ones that needs schema. In /core/config/schema/core.entity.schema.yml, the schema for the entity reference field formatters are defined:

field.formatter.settings.entity_reference_entity_view:
  type: mapping
  label: 'Entity reference rendered entity display format settings'
  mapping:
    view_mode:
      type: string
      label: 'View mode'
    link:
      type: boolean
      label: 'Show links'

field.formatter.settings.entity_reference_entity_id:
  type: mapping
  label: 'Entity reference entity ID display format settings'

field.formatter.settings.entity_reference_label:
  type: mapping
  label: 'Entity reference label display format settings'
  mapping:
    link:
      type: boolean
      label: 'Link label to the referenced entity'

While we could copy-paste these definitions for our field formatters, I believe it is enough to just "extend" the formatters above. That would result into something like this (I hope I got the keys for Feeds right):

field.formatter.settings.feeds_item_target_entity_view:
  type: field.formatter.settings.entity_reference_entity_view

field.formatter.settings.feeds_item_target_id:
  type: field.formatter.settings.entity_reference_entity_id

field.formatter.settings.feeds_item_target_label:
  type: field.formatter.settings.entity_reference_label
jamesdixon’s picture

Thanks that fixed it MegaChriz!

Tests are getting closer. For FeedsItemTargetEntityFormatterTest.php

this->createFieldWithStorage()

Is set up to create a field on the article node by default. Next step is for me to figure out how to set it on the feeds item instead. Gotta mess with that settings array, shouldn't be too bad.

jamesdixon’s picture

Sorry last patch had a swp file issue.

jamesdixon’s picture

I'm running into an issue where the new oneliner field I created is not being rendered along with the other feeds item fields of next import, guid, etc.

When I var_dump() the $article->feeds_item, it is not showing the oneliner field anywhere. I added the oneliner field to the feed, and now I'm thinking maybe I need to add it to the feeds_items directly somehow.

megachriz’s picture

@jamesdixon
$article->feeds_item only holds a reference to a feeds_feed entity, it does not contain the oneliner field from your test. That field should live on the feeds_feed entity. The field should not be added to feeds_item.

+++ b/tests/src/Functional/Plugin/Field/FieldFormatter/FeedsItemTargetEntityFormatterTest.php
@@ -0,0 +1,87 @@
+    $feed->oneliner = [
+      'value' => '<p>He is not only from medieval Japan, but also from an alternate universe, so naturally he speaks English!</p>',
+      'format' => 'full_html',
+    ];

If you don't get the field rendered, could it be that $feed needs to be saved after setting the oneliner field?

Else, I would try first to - on a real Drupal install - add a text field to a feed type, create a feed, then visit the feed page to see if the field gets displayed there.

megachriz’s picture

StatusFileSize
new73.53 KB
new21.02 KB

@jamesdixon
I got the following error when running the test:

InvalidArgumentException: Field oneliner is unknown.

That's because the feed object doesn't have that field yet at the moment that it is created. This can be fixed by reloading the feed object:

$feed = $this->reloadEntity($feed);

To test if the field was actually created and with a value, I temporary added the following to the test:

$this->drupalGet('feed/1');

This reveals how the field is displayed on - in this case - the feed entity itself.

If you are running tests locally and no HTML is generated, be sure to change the following in phpunit.xml:

  • Add option printerClass="\Drupal\Tests\Listeners\HtmlOutputPrinter":
    <phpunit bootstrap="core/tests/bootstrap.php" colors="true"
             beStrictAboutTestsThatDoNotTestAnything="true"
             beStrictAboutOutputDuringTests="true"
             beStrictAboutChangesToGlobalState="true"
             printerClass="\Drupal\Tests\Listeners\HtmlOutputPrinter"
             checkForUnintentionallyCoveredCode="false">
    
  • Set a browser output directory:
    <env name="BROWSERTEST_OUTPUT_DIRECTORY" value="/Users/youri/Sites/devdesktop/drupal8/sites/simpletest/browser_output"/>
    

To test how the field looks like in the admin interface, I temporary added the following code to the test:

$temporary_secretary = $this->drupalCreateUser([
  'administer feeds',
  'access feed overview',
  'administer feeds_feed fields',
  'administer filters',
]);
$this->drupalLogin($temporary_secretary);
$this->drupalGet('/admin/structure/feeds/manage/' . $feed_type_id  . '/fields');
$this->drupalGet('/admin/structure/feeds/manage/' . $feed_type_id  . '/fields/feeds_feed.' . $feed_type_id . '.oneliner');
$this->drupalGet('/admin/config/content/formats');

And I added 'field_ui' to the $modules list in FeedsItemFormatterTestBase.

This revealed that there is no text format 'full_html', only 'plain_text':


mpp’s picture

jamesdixon’s picture

Thanks for all the support @megachriz! I was able to get the FeedsItemTargetEntityFormatterTest working with your guidance.

Posting the updated test and I've got one test to go (FeedsItemTargetLabelFormatterTest.php).

jamesdixon’s picture

So the FeedsItemTargetLabelFormatterTest is giving me some unusual output, but maybe that's how it's supposed to work:

  <span class="field field--name-title field--type-string field--label-hidden">Highlander</span>
  <span class="field field--name-uid field--type-entity-reference field--label-hidden"><span>Anonymous (not verified)</span></span>\n
  <span class="field field--name-created field--type-created field--label-hidden">Sat, 04/13/2019 - 06:17</span>
  <div class="field field--name-feeds-item field--type-feeds-item field--label-above">
     <div class="field__label">feeds_item label</div>\n
     <div class="field__item"><a href="/feed/1" hreflang="und">qSlf3Zx8</a></div>
   </div>

When I set the label to "Witty one liner label" I expected the markup would contain:

<div class="field__item"><a href="/feed/1" hreflang="und">Witty one liner label</a></div>

Instead it is giving me weird hash inside the link element:

  <div class="field__item"><a href="/feed/1" hreflang="und">qSlf3Zx8</a></div>

Is this how labels are supposed to be rendered when we use the FeedsItemTargetLabelFormatter?

I wrote the test assuming this is right.

jamesdixon’s picture

Status: Needs work » Needs review

The last submitted patch, 53: feeds-feeds-item-formatters-2799225-53.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 43: feeds-feeds-item-formatters-2799225-43.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 54: feeds-feeds-item-formatters-2799225-54.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 55: feeds-feeds-item-formatters-2799225-55.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 80: feeds-feeds-item-formatters-2799225-80.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 68: feeds-feeds-item-formatters-2799225-68.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 85: feeds-feeds-item-formatters-2799225-85.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 77: feeds-feeds-item-formatters-2799225-76.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 74: feeds-feeds-item-formatters-2799225-74.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 57: feeds-feeds-item-formatters-2799225-57.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 86: feeds-feeds-item-formatters-2799225-86.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

jamesdixon’s picture

Wow something odd happened there. It submitted all the patches in the issue for testing.

Looks like it doesn't like how I used incomplete markup as an expected result:

$expected = '<div class="field__item"><a href="/feed/1" hreflang="und">';
megachriz’s picture

Hm, the test Drupal\Tests\feeds\Functional\Plugin\Field\FieldFormatter\FeedsItemTargetLabelFormatterTest::testFeedsItemTargetLabelFormatterLink is passing locally.

Adding a htmlOutput() call so we can check the HTML on the dispatcher (Build artifacts > artifacts > run_tests.js > simpletest_html). For the latest patch, see for example https://dispatcher.drupalci.org/job/drupal8_contrib_patches/55092/artifa...

I also did some small refactoring. FeedsItemTargetEntityFormatterTest::addFieldToFeed() and FeedsItemTargetLabelFormatterTest::addFieldToFeed() were exactly the same, so I added that method to FeedsItemFormatterTestBase instead.

megachriz’s picture

Status: Needs work » Needs review
StatusFileSize
new33.45 KB
new1.55 KB

Removing unused use statements.

Status: Needs review » Needs work

The last submitted patch, 102: feeds-feeds-item-formatters-2799225-102.patch, failed testing. View results

megachriz’s picture

The Drupal testbot uses a Drupal install in a subdirectory and is therefore outputting the following:
<a href="/subdirectory/feed/1" hreflang="und">dZXhZpHI</a>
See https://dispatcher.drupalci.org/job/drupal8_contrib_patches/56229/artifa...

So in the test we need to build a link somehow that takes the current install location into account.

megachriz’s picture

Status: Needs work » Needs review
StatusFileSize
new33.84 KB
new2.24 KB

I've modified the test FeedsItemTargetLabelFormatterTest::testFeedsItemTargetLabelFormatterLink(): generating the expected url is now done in the same way as the test Drupal\Tests\field\Kernel\EntityReference\EntityReferenceFormatterTest::testLabelFormatter() is doing. Passes locally at least.

jamesdixon’s picture

Assigned: jamesdixon » Unassigned
Status: Needs review » Reviewed & tested by the community

Good call on moving the addFieldtoFeed() method into the FeedsBrowserTestBase.

Ah okay the subdirectory issue was tripping us up. Thanks for fixing that part.

I have run the tests locally and they pass for me also.

Your code updates look good to me! Hopefully we're good to wrap this one up. :)

jamesdixon’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new33.69 KB
new2.25 KB

I've cleaned up some variables which are instantiated and only used once as suggested by @megachriz.

megachriz’s picture

Thanks, James! I went through all the code one more time and did a few cleanups. Hopefully I didn't break the tests. I also corrected docs in FeedCreationTrait.

If this passes, then I'l commit it!

  • MegaChriz committed 80dd150 on 8.x-3.x authored by jamesdixon
    Issue #2799225 by jamesdixon, MegaChriz, jcnventura, vijay.mayilsamy,...
megachriz’s picture

Status: Needs review » Fixed

Alright, committed #108!

Thanks to João for the initial idea and work, James for writing the tests and additional formatters, Andy and Vijay for testing/reviewing.

@jcnventura
It took us a while, but now you can finally continue with #3008991: Migrate feeds content from Drupal 7!

jcnventura’s picture

Yay! Well done everyone :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jnicola’s picture

Hey there, so we're using feeds 8.x-3.0-beta4, which should have this code in it... but I get the following error still when trying to drag the widget out of hidden into display so I can try and troubleshoot it.

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "hidden" plugin does not exist.

It appears to me that this issue may not be resolved? This project is also far newer than the 4 years ago that this was resolved (2 years old now) so... not sure what is missing?

NOTE: Just tested with 8.x-3.x-dev. Same issue?

megachriz’s picture

@jnicola
I think it was only fixed for display formatters (that can be configured on "Manage display" pages) and not for widgets (on "Manage form display" pages). Can you create a new issue for it?