Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Assigned: Unassigned » swentel

I'll do this tonight, shouldn't be hard

swentel’s picture

Status: Active » Needs work
FileSize
5.17 KB

Still needs work, the flagErrors doesn't work at all

swentel’s picture

Project: D8 Field API » Drupal core
Version: » 8.x-dev
Component: Code » field system
Status: Needs work » Needs review
FileSize
4.69 KB

And the patch.

It's kind of annoying to have to leave the validate in the module file. Maybe we should add a validate method ?

swentel’s picture

Status: Needs review » Needs work
netsensei’s picture

Converted the formatters too. The #element_validate callback still needs to be added.

netsensei’s picture

Status: Needs work » Needs review
FileSize
20.28 KB

Now with added #element_validate callback.

sun’s picture

This looks really great.

+class LinkSeparateFormatter extends LinkFormatter {


-function link_field_formatter_info() {
...
-  // @todo Merge into 'link' formatter once there is a #type like 'item' that
-  //   can render a compound label and content outside of a form context.
-  $formatters['link_separate'] = array(
-    'label' => t('Separate title and URL'),

We should keep this @todo and move it into the phpDoc of the LinkSeparateFormatter class instead.

Apparently, there was no issue for this yet, so I've created one: #1829202: Make #type 'item' work outside of a form context to render a compound label + content

netsensei’s picture

nils.destoop’s picture

Title: Convert Link/URL widgets to plugin system » Convert Link/URL widgets / formatters to plugin system
nils.destoop’s picture

Had some notices when using the seperate formatter, also removed an unused setting.
Also added an $this->getPluginId() == 'link' check. Unexisting settings where shown in the summary, when you switched formatter.

falcon03’s picture

To solve
http://drupal.org/node/1801268
I have to do some work on the link widget. So, just to avoid conflicts, should I proceed with those improvements or should I wait for this patch to get in?

@zuuperman: Could you tell me which unused setting did you remove in the patch?

nils.destoop’s picture

If you first configured the formatter to be shown as link with the setting: 'Show URL only'. 'Show URL only' was still shown when you switched to the seperate link formatter, although that setting doesn't exist for that formatter.

netsensei’s picture

Status: Needs review » Needs work

Hm. Sad panda. Patch does not apply any more. :-(

  • Link module has been moved to core/modules from core/modules/field/modules.
  • We need to integrate #1241938-48: Add support for #placeholder to relevant Field API widgets and any other general field API patches in this patch now.
  • netsensei’s picture

    Okay. I've rerolled the patch with the integration of #1241938: Add support for #placeholder to relevant Field API widgets.

    Simpletest breaks on my installation though: Fatal error: Call to a member function entityType() on a non-object in /www/core/modules/link/lib/Drupal/link/Tests/LinkFieldTest.php on line 474.

    I don't have the time to investigate any deeper at this time. :-(

    netsensei’s picture

    Forgot the patch.

    andypost’s picture

    Status: Needs work » Needs review

    Status: Needs review » Needs work

    The last submitted patch, 1796316-14-link-formatter-widget.patch, failed testing.

    Dave Reid’s picture

    Component: field system » link.module
    netsensei’s picture

    Status: Needs work » Needs review
    FileSize
    25 KB

    Okay. New attempt. This one conversion works better. No exceptions. But still a some tests it will fail:

    Raw "http://www.example.com/content/articles/archive?author=John&year=2012#com" found Other LinkFieldTest.php 309 Drupal\link\Tests\LinkFieldTest->testLinkFormatter()

    Raw "A very long & strange example title that could break the nice layout of the site" found Other LinkFieldTest.php 313 Drupal\link\Tests\LinkFieldTest->testLinkFormatter()

    Almost there!

    Status: Needs review » Needs work

    The last submitted patch, 1796316-19-link-conversion.patch, failed testing.

    swentel’s picture

    Status: Needs work » Needs review
    FileSize
    24.63 KB

    You apparently had quite old version of core :)

    Sorry for not having an interdiff: changes

    • Annotions namespace changed
    • 'Definition of' is not 'Contains'
    • remove link_field_prepare_view() and merge with prepareView in the formatter

    Lot's of failure, will look further.

    swentel’s picture

    +++ b/core/modules/link/lib/Drupal/link/Plugin/field/formatter/LinkSeparateFormatter.phpundefined
    @@ -0,0 +1,81 @@
    +        // Unsanitizied token replacement here because $options['html'] is FALSE
    

    'Unsanitizied' typo

    Status: Needs review » Needs work

    The last submitted patch, 1796316-21-link-conversion.patch, failed testing.

    swentel’s picture

    Ok, looks like the link_field_prepare_view() messed some stuff up. I don't have time anymore until april 7 to get this one home though.

    klonos’s picture

    Assigned: swentel » Unassigned

    ... ;)

    netsensei’s picture

    Assigned: Unassigned » netsensei

    :-)

    netsensei’s picture

    Status: Needs work » Needs review
    FileSize
    25.51 KB

    Okay. Found out what was missing:

    * You were right about the prepareView stuff. $this->getSettings wasn't called in prepareView
    * In viewElements, $item['url'] was assigned instead of $item['path']

    This one should be green across the board! :-) Go testbot!

    swentel’s picture

    Sweet - filed #1955678: Remove formatter and widgets legacy plugin - we can remove those in a separate issue.

    nils.destoop’s picture

    General

    @file descriptions with contains should start with \
    Example:

    /**
     * @file
     * Contains \Drupal\link\Plugin\field\widget\LinkWidget.
     */
    

    When declaring your plugin settings, don't end with a comma. (LinkFormatter, LinkSeparateFormatter, LinkWidget).

    example:

     *   settings = {
     *     "trim_length" = "80",
     *     "url_only" = "",
     *     "url_plain" = "",
     *     "rel" = "",
     *     "target" = "",
     *   }
    

    The comma after target should be removed.

    LinkSeperatorFormatter

    line 68. There is 2 times the same code:

          if (!empty($settings['trim_length'])) {
            $url_title = truncate_utf8($item['url'], $settings['trim_length'], FALSE, TRUE);
          }
    

    LinkWidget

    In the new widget class, this is the description from placeholder_url and placeholder_title:

    t('The placeholder is a short hint (a word or short phrase) intended to aid the user with data entry. A hint could be a sample value or a brief description of the expected format.')
    

    The patch says:

    -    '#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.'),
    

    Looks like your widget still contains an older description.

    After changing those remarks, it's RTBC for me.

    netsensei’s picture

    @file descriptions with contains should start with \

    Done.

    When declaring your plugin settings, don't end with a comma. (LinkFormatter, LinkSeparateFormatter, LinkWidget).

    Done.

    line 68. There is 2 times the same code:

    Fixed. Removed the excess if condition.

    Looks like your widget still contains an older description.

    Changed this to read the new statement:

    '#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.'),

    Patch attached.

    netsensei’s picture

    FileSize
    4.1 KB

    Ah yes, forgot the interdiff.txt.

    nils.destoop’s picture

    Status: Needs review » Reviewed & tested by the community

    :)

    amateescu’s picture

    Status: Reviewed & tested by the community » Needs work
    +++ b/core/modules/link/lib/Drupal/link/Plugin/field/formatter/LinkFormatter.php
    @@ -0,0 +1,184 @@
    +use Drupal\Core\Annotation\Translation;
    +use Drupal\field\Plugin\Type\Formatter\FormatterBase;
    +use Drupal\Core\Entity\EntityInterface;
    

    The patch needs to be rerolled anyway for the problem below, so while you're there, could you please move these in alphabetical order? Meaning that Drupal\Core\Entity\EntityInterface goes between Drupal\Core\Annotation\Translation and Drupal\field\Plugin\Type\Formatter\FormatterBase.

    +++ b/core/modules/link/link.module
    @@ -15,7 +15,7 @@ function link_help($path, $arg) {
    -      $output .= '<p>' . t('The Link module defines a simple link field type for the Field module. Links are external URLs, can have an optional title for each link, and they can be formatted when displayed. See the <a href="@field-help">Field module help page</a> for more information about fields.', array('@field-help' => url('admin/help/field'))) . '</p>';
    +      $output .= '<p>' . t('The Link modulfe defines a simple link field type for the Field module. Links are external URLs, can have an optional title for each link, and they can be formatted when displayed. See the <a href="@field-help">Field module help page</a> for more information about fields.', array('@field-help' => url('admin/help/field'))) . '</p>';
    

    Wat? We have the notion of 'modulfe' in Drupal now? :P

    netsensei’s picture

    Status: Needs work » Needs review
    FileSize
    2.59 KB
    24.55 KB

    Fixed! Happy Easter! ;-)

    amateescu’s picture

    The interdiff looks good, but I think you uploaded an older version of the patch that doesn't contain the 'modulfe' fix. Happy Easter indeed! :)

    netsensei’s picture

    Okay. New attempt. On my topical branch, I introduced the typo in one commit, but removed 2 commits later. My bad. The typo diff shouldn't be showing up at all as HEAD is looking fine all along. ;-)

    New untainted patch attached. This should be it.

    amateescu’s picture

    Status: Needs review » Reviewed & tested by the community

    Looks good now.

    dawehner’s picture

    That's a really fast drive by review, so that's all minor.

    +++ b/core/modules/link/lib/Drupal/link/Plugin/field/formatter/LinkFormatter.phpundefined
    @@ -0,0 +1,184 @@
    +   * Implements Drupal\field\Plugin\Type\Formatter\FormatterInterface::settingsForm().
    

    Missing "\". Btw. This overrides FormatterBase::settingsForm().

    +++ b/core/modules/link/lib/Drupal/link/Plugin/field/formatter/LinkFormatter.phpundefined
    @@ -0,0 +1,184 @@
    +  public function settingsForm(array $form, array &$form_state) {
    

    As it overrides the parent, afaik it should call the parent just because out of habit.

    +++ b/core/modules/link/lib/Drupal/link/Plugin/field/formatter/LinkFormatter.phpundefined
    @@ -0,0 +1,184 @@
    +   * Implements Drupal\field\Plugin\Type\Formatter\FormatterInterface::settingsSummary().
    

    "\" here and on all other methods.

    +++ b/core/modules/link/lib/Drupal/link/Plugin/field/formatter/LinkSeparateFormatter.phpundefined
    @@ -0,0 +1,79 @@
    +  }
    +}
    

    Here should be an empty line.

    netsensei’s picture

    Status: Reviewed & tested by the community » Needs work

    Okay. Going to fix those tommorow. Thx for the feedback!

    netsensei’s picture

    Okay. Made these changes:

    • Added \ in the comments for the methods
    • Added: parent::settingsForm($form, $form_state) in LinkFormatter
    • Added: parent::settingsForm($form, $form_state) in LinkWidget
    • Added: missing white line in LinkSeparateFormatter
    • Added: missing white line in LinkWidget

    I've noticed that other formatters like Number are not adhering to those standards yet. We need a separate clean up task for those.

    Go testbot!

    netsensei’s picture

    Status: Needs work » Needs review

    Status: Needs review » Needs work

    The last submitted patch, 1796316-40-link-conversion.patch, failed testing.

    netsensei’s picture

    Status: Needs work » Needs review
    FileSize
    23.8 KB

    Hm. My mistake.

    andypost’s picture

    Status: Needs review » Reviewed & tested by the community

    Looks all from #38 addressed

    swentel’s picture

    Status: Reviewed & tested by the community » Postponed

    We're going to postpone on #1735118: Convert Field API to CMI, that one is close, bear with us folks!

    yched’s picture

    Status: Postponed » Reviewed & tested by the community

    Un-postponing.

    yched’s picture

    #43: 1796316-43-link-conversion.patch queued for re-testing.

    netsensei’s picture

    Does this mean that the patch van be comitted as is, or do we have to rework it?

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 1796316-43-link-conversion.patch, failed testing.

    netsensei’s picture

    Status: Needs work » Reviewed & tested by the community

    Yup. Expected. :-) I have all day tommorrow to revisit this one.

    netsensei’s picture

    Status: Reviewed & tested by the community » Needs work
    jibran’s picture

    Status: Needs work » Needs review
    FileSize
    24.24 KB

    Here is the re-roll #1735118: Convert Field API to CMI didn't touch link .module. Conflict was #1957670-10: Link field labels don't show in entity forms

    +  // If cardinality is 1, ensure a label is output for the field by wrapping it
    +  // in a details element.
    +  if ($field['cardinality'] == 1) {
    +    $element += array(
    +      '#type' => 'fieldset',
    +    );
    +  }
    +

    above is moved into core/modules/link/lib/Drupal/link/Plugin/field/widget/LinkWidget.php

    +    // If cardinality is 1, ensure a label is output for the field by wrapping it
    +    // in a details element.
    +    if ($this->field['cardinality'] == 1) {
    +      $element += array(
    +        '#type' => 'fieldset',
    +      );
    +    }
    +

    Status: Needs review » Needs work

    The last submitted patch, 1796316-52-link-conversion.patch, failed testing.

    jibran’s picture

    Status: Needs work » Needs review

    #52: 1796316-52-link-conversion.patch queued for re-testing.
    seems unrelated checked on local passes just fine.

    Message Group Filename Line Function Status
    Invalid values generate a list of form errors. Other EntityTranslationUITest.php 40 Drupal\translation_entity\Tests\EntityTranslationUITest->testTranslationUI()
    yched’s picture

    Status: Needs review » Needs work

    According to latest core standards, all method phpdocs that consist only of "Implements Foo::bar()" or "Overrides Foo::bar()" should now just be :

      /**
       * {@inheritdoc}
       */
    
    jibran’s picture

    Status: Needs work » Needs review
    FileSize
    3.21 KB
    23.78 KB

    Done.

    yched’s picture

    Status: Needs review » Reviewed & tested by the community

    Thanks !

    alexpott’s picture

    #56: 1796316-56-link-conversion.patch queued for re-testing.

    yched’s picture

    yched’s picture

    #56: 1796316-56-link-conversion.patch queued for re-testing.

    tstoeckler’s picture

    Priority: Normal » Major

    The priority was never actually bumped, so per #59 doing that now.

    alexpott’s picture

    Status: Reviewed & tested by the community » Needs work

    Needs a re-roll error: patch failed: core/modules/link/link.module:94

    jibran’s picture

    Status: Needs work » Needs review
    FileSize
    1005 bytes
    23.8 KB
    swentel’s picture

    Status: Needs review » Reviewed & tested by the community

    Assuming it's green.

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 1796316-63.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Reviewed & tested by the community
    FileSize
    1.04 KB
    23.81 KB

    Now it will be green.

    swentel’s picture

    FileSize
    1.04 KB
    23.81 KB

    Another token_replace() left over.

    swentel’s picture

    nice cross post :)

    alexpott’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed 1ad0ade and pushed to 8.x. Thanks!

    Status: Fixed » Closed (fixed)

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