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.
Comment | File | Size | Author |
---|---|---|---|
#67 | 1796316-66.patch | 23.81 KB | swentel |
#67 | interdiff.txt | 1.04 KB | swentel |
#66 | 1796316-66.patch | 23.81 KB | amateescu |
#66 | interdiff.txt | 1.04 KB | amateescu |
#63 | 1796316-63.patch | 23.8 KB | jibran |
Comments
Comment #1
swentel CreditAttribution: swentel commentedI'll do this tonight, shouldn't be hard
Comment #2
swentel CreditAttribution: swentel commentedStill needs work, the flagErrors doesn't work at all
Comment #3
swentel CreditAttribution: swentel commentedAnd the patch.
It's kind of annoying to have to leave the validate in the module file. Maybe we should add a validate method ?
Comment #4
swentel CreditAttribution: swentel commented#1805688: Support methods as #element_validate callbacks is in, let's use that.
Comment #5
netsensei CreditAttribution: netsensei commentedConverted the formatters too. The #element_validate callback still needs to be added.
Comment #6
netsensei CreditAttribution: netsensei commentedNow with added #element_validate callback.
Comment #7
sunThis looks really great.
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
Comment #8
netsensei CreditAttribution: netsensei commentedDone!
Comment #9
nils.destoop CreditAttribution: nils.destoop commentedComment #10
nils.destoop CreditAttribution: nils.destoop commentedHad 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.
Comment #11
falcon03 CreditAttribution: falcon03 commentedTo 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?
Comment #12
nils.destoop CreditAttribution: nils.destoop commentedIf 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.
Comment #13
netsensei CreditAttribution: netsensei commentedHm. Sad panda. Patch does not apply any more. :-(
Comment #14
netsensei CreditAttribution: netsensei commentedOkay. 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. :-(
Comment #15
netsensei CreditAttribution: netsensei commentedForgot the patch.
Comment #16
andypostComment #18
Dave ReidComment #19
netsensei CreditAttribution: netsensei commentedOkay. 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!
Comment #21
swentel CreditAttribution: swentel commentedYou apparently had quite old version of core :)
Sorry for not having an interdiff: changes
Lot's of failure, will look further.
Comment #22
swentel CreditAttribution: swentel commented'Unsanitizied' typo
Comment #24
swentel CreditAttribution: swentel commentedOk, 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.
Comment #25
klonos... ;)
Comment #26
netsensei CreditAttribution: netsensei commented:-)
Comment #27
netsensei CreditAttribution: netsensei commentedOkay. 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!
Comment #28
swentel CreditAttribution: swentel commentedSweet - filed #1955678: Remove formatter and widgets legacy plugin - we can remove those in a separate issue.
Comment #29
nils.destoop CreditAttribution: nils.destoop commentedGeneral
@file descriptions with contains should start with \
Example:
When declaring your plugin settings, don't end with a comma. (LinkFormatter, LinkSeparateFormatter, LinkWidget).
example:
The comma after target should be removed.
LinkSeperatorFormatter
line 68. There is 2 times the same code:
LinkWidget
In the new widget class, this is the description from placeholder_url and placeholder_title:
The patch says:
Looks like your widget still contains an older description.
After changing those remarks, it's RTBC for me.
Comment #30
netsensei CreditAttribution: netsensei commentedDone.
Done.
Fixed. Removed the excess if condition.
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.
Comment #31
netsensei CreditAttribution: netsensei commentedAh yes, forgot the interdiff.txt.
Comment #32
nils.destoop CreditAttribution: nils.destoop commented:)
Comment #33
amateescu CreditAttribution: amateescu commentedThe 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 betweenDrupal\Core\Annotation\Translation
andDrupal\field\Plugin\Type\Formatter\FormatterBase
.Wat? We have the notion of 'modulfe' in Drupal now? :P
Comment #34
netsensei CreditAttribution: netsensei commentedFixed! Happy Easter! ;-)
Comment #35
amateescu CreditAttribution: amateescu commentedThe interdiff looks good, but I think you uploaded an older version of the patch that doesn't contain the 'modulfe' fix. Happy Easter indeed! :)
Comment #36
netsensei CreditAttribution: netsensei commentedOkay. 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.
Comment #37
amateescu CreditAttribution: amateescu commentedLooks good now.
Comment #38
dawehnerThat's a really fast drive by review, so that's all minor.
Missing "\". Btw. This overrides FormatterBase::settingsForm().
As it overrides the parent, afaik it should call the parent just because out of habit.
"\" here and on all other methods.
Here should be an empty line.
Comment #39
netsensei CreditAttribution: netsensei commentedOkay. Going to fix those tommorow. Thx for the feedback!
Comment #40
netsensei CreditAttribution: netsensei commentedOkay. Made these changes:
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!
Comment #41
netsensei CreditAttribution: netsensei commentedComment #43
netsensei CreditAttribution: netsensei commentedHm. My mistake.
Comment #44
andypostLooks all from #38 addressed
Comment #45
swentel CreditAttribution: swentel commentedWe're going to postpone on #1735118: Convert Field API to CMI, that one is close, bear with us folks!
Comment #46
yched CreditAttribution: yched commentedUn-postponing.
Comment #47
yched CreditAttribution: yched commented#43: 1796316-43-link-conversion.patch queued for re-testing.
Comment #48
netsensei CreditAttribution: netsensei commentedDoes this mean that the patch van be comitted as is, or do we have to rework it?
Comment #50
netsensei CreditAttribution: netsensei commentedYup. Expected. :-) I have all day tommorrow to revisit this one.
Comment #51
netsensei CreditAttribution: netsensei commentedComment #52
jibranHere 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
above is moved into
core/modules/link/lib/Drupal/link/Plugin/field/widget/LinkWidget.php
Comment #54
jibran#52: 1796316-52-link-conversion.patch queued for re-testing.
seems unrelated checked on local passes just fine.
Comment #55
yched CreditAttribution: yched commentedAccording to latest core standards, all method phpdocs that consist only of "Implements Foo::bar()" or "Overrides Foo::bar()" should now just be :
Comment #56
jibranDone.
Comment #57
yched CreditAttribution: yched commentedThanks !
Comment #58
alexpott#56: 1796316-56-link-conversion.patch queued for re-testing.
Comment #59
yched CreditAttribution: yched commentedBumping priority, this is now a blocker for #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets.
Comment #60
yched CreditAttribution: yched commented#56: 1796316-56-link-conversion.patch queued for re-testing.
Comment #61
tstoecklerThe priority was never actually bumped, so per #59 doing that now.
Comment #62
alexpottNeeds a re-roll
error: patch failed: core/modules/link/link.module:94
Comment #63
jibranConflict with #1969540: Convert token.inc to a service.
Comment #64
swentel CreditAttribution: swentel commentedAssuming it's green.
Comment #66
amateescu CreditAttribution: amateescu commentedNow it will be green.
Comment #67
swentel CreditAttribution: swentel commentedAnother token_replace() left over.
Comment #68
swentel CreditAttribution: swentel commentednice cross post :)
Comment #69
alexpottCommitted 1ad0ade and pushed to 8.x. Thanks!