Apply formatters and widgets to Feed base fields

Assigning to me to resolve the test failures during the Szeged sprint.

Fixed the test failures.

Fixed the remaining test failures.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Entity/Feed.php
@@ -134,7 +134,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setDisplayConfigurable('form', TRUE);

Note that 'title' was not exposed as an 'extra field' in HEAD, and so was not configurable (i.e eligible for drag-n-drop reorder) in Field UI "Manage forms" so far.

Probably no biggie, just pointing that this is a functionnal change.

Good point, but we simply didn't bother with extra fields when making feeds fieldable, so this seems fine with me. It wasn't explicitly not added.

Here's my attempt at converting the URL form element to a base field. It also adds a 'uri' widget. I'm adding it as 'do-not-test' because relevant tests have not been adjusted yet, but I still wanted to share the progress.

Maybe we could somehow use #2054011: Implement built-in support for internal URLs for that? Would be quite a bit an overhead just to store the URL though, so maybe not.

Re-triggering tests on last patch...

Oops, didnt see the 'do-not-test'. My mistake!

Re #12: Maybe I didn't understand your question correctly, but usually feed URL's would be external, not internal, right?

After having had a look at the UI, there doesn't seem to be an admin page for configuring feed fields (I may be wrong, I have never actually used the aggregate module).

Therefore I have made the title and url non-configurable.

Also the patch in #11 doesn't 'create' the file, UriWidget, but modifies a non-existent file... (not sure how that happened) so I fixed that in the patch.

Thanks for reviewing, yanniboi.

Here's a new patch and interdiff, including tests for the url field.

ToDo: convert the interval field.

I am attempting to convert the interval field.

I'm not sure if @marcvangend is actually already working on the interval field so unassigning myself...

New patch. This one includes the field for the refresh interval. I also made sure that the order of fields and all labels and descriptions exactly match the current situation.

My bad. Try again.

One small nitpick:

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Entity/Feed.php
@@ -151,11 +157,27 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+
+    $intervals = array(900, 1800, 3600, 7200, 10800, 21600, 32400, 43200, 64800, 86400, 172800, 259200, 604800, 1209600, 2419200);
+    $period = array_map('format_interval', array_combine($intervals, $intervals));
+    $period[AGGREGATOR_CLEAR_NEVER] = t('Never');
+

You add the allowed values here.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.php
@@ -27,35 +27,14 @@ public function form(array $form, array &$form_state) {
     $period[AGGREGATOR_CLEAR_NEVER] = $this->t('Never');

but forget to delete them here...

I'll quickly patch that because its not really a big deal.

Not testing because I haven't fixed the test failures.

I spent quite a while looking at DependencyTest, and I have no idea why our patch would change the order of the expected_modules variable, but the test is out of date anyway (and needs a follow up issue to look at it). This should pass tests.... (Fingers crossed)

Yeah, DependencyTest is the pain.

This looks great, once we have the language widget we can completely remove the form method. Not sure what's messing with the user permission test, maybe the form isn't being submitted?

Good catch in #26.

The DependencyTest must have changed because aggregator module now lists options module as a dependency, because it has a select widget on the form.

I'll try to fix the permission test.

Ok, @marcvangend and I had a look at the UserPermissionsTest and @Berdir suggested we replace the module enable form submit with a ModuleHandler()->install() function because that handles dependencies by default. We think this is OK because the test isn't concerned with module dependencies, but the permissions resulting from them.

Patch attached!

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.php
@@ -23,39 +23,15 @@ class FeedFormController extends ContentEntityFormController {
+    // Blocked on https://drupal.org/node/2226493 which adds a generic language widget.

more then 80 chars and 2nd line after @todo starts after t of @todo :). See 1354#todo.

New patch. This fixes the points from #34 and #35. I didn't run all the tests again because I have a plane to catch, but I guess it will be fine :-)

Sorry, forget #38, I missed a file.

Only list_integer has support for allowed values, so I guess we need to make that switch.

We're working on unifying the field types in #2150511: [meta] Deduplicate the set of available field types, but I'm not sure if this is subject to that. Would be nice to only have integer field type and make options.module just provide widgets, but not sure if that's feasible.

list_integer is not duplicating any other field type, so it is not in the current scope of #2150511: [meta] Deduplicate the set of available field types.

IIRC, one of the last blockers for moving list field types into Core/Field (aside from list_boolean, which is a bit aside and has its own issue already) is #2171397: Move options_allowed_values() to a method somewhere ?. @andypost is probably more up-to-speed on that topic that I have been lately :-/

True, it's not an exact duplicate, that's why I said it's not really in the scope of that :)

*But*, I find the name misleading (it's not about having a list of integers, but an integer field with a list of allowed values and select/options/checkboxes widgets) and it woud be nice if we could have just a single integer field type. Anyway, not a huge problem.

name: true :-/
single integer field type: the problem is that a 'select' or 'checkbox' widget can only be applied to a field type that has a finite set of allowed values. If we have a single int field type that can be either opened- or closed-values, then it becomes fuzzy to decide when the select widget is applicable. Widgets are only eligible by field type.

Re #46, I mainly guided myself to that solution, wondering if it was really needed to add a new widget while we already have a select widget (you know, DRY). Fago helped me when I got stuck on some typeddata error, he did not express an opinion about this specific solution.

Yeah, I think this is the preferred way as well. Then we have this information available through the field definition and can validate it when entities are created in a different way instead of relying on a special widget.

Straight re-roll.

The patch in #52 removes one of the lines (introduced in #2177799: Allow IntegerItem's to be unsigned) that made the re-roll necessary in the first place. I'm adding it back in.

@mr.baileys, please try to create interdiffs, so small mistakes like this are easier to spot.

@marcvanged: apologies... Didn't know you could create an interdiff for rerolls btw...

@mr.baileys: no apologies needed, thanks for your help.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/UriWidget.php
   @@ -0,0 +1,85 @@
   +      '#title' => t('Size of URI field'),
   ...
   +      '#title' => t('Placeholder'),
   ...
   +      '#description' => t('Text that will be shown inside the field until a value is entered. This hint is usually a sample value or a brief description of the expected format.'),
   ...
   +    $summary[] = t('URI field size: !size', array('!size' => $this->getSetting('size')));
   ...
   +      $summary[] = t('Placeholder: @placeholder', array('@placeholder' => $placeholder));
   

   $this->t()

2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/UpdateFeedTest.php
   @@ -23,7 +23,7 @@ public static function getInfo() {
   -    $remamining_fields = array('title', 'url', '');
   +    $remamining_fields = array('title[0][value]', 'url[0][value]', '');
        foreach ($remamining_fields as $same_field) {
   

   Let's fix the spelling mistake considering you are touching the line.

i have updated the patch based on #57.

Committed/pushed to 8x, thanks!

No auto-commit message, must not have pushed

Committed af9cd0f and pushed to 8.x. Thanks!

Filed follow-up #2236599: Inherit UriWidget from StringTextfieldWidget

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Entity/Feed.php
@@ -152,12 +158,28 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setSetting('max_length', NULL)

is wrong, 'uri' field type sets to 2048 but the schema defines 'text' so there's no way to build schema from this definition

Changed the data type for uri to varchar.

#66 does not help here, base table defines 'text' (unlimited) about varchar has limits

We are now generating the schema, so I assume whatever fix was necessary for this was done already. Closing again.