Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Sub issue of #2014671: [META] Convert all field types to plugins.
Looks like poor field_test.module got forgotten, so here's a start. Have moved a few things over, validation isn't adjusted yet and some crazy parts with that memorize stuff aren't either. I think it makes sense to re-think that a bit. Looks like basic crud tests worked, let's see about the others.
Related change notice https://drupal.org/node/2064123
Comment | File | Size | Author |
---|---|---|---|
#41 | interdiff.txt | 564 bytes | andypost |
#41 | field-test-type-2083803-41.patch | 27.12 KB | andypost |
#39 | interdiff.txt | 982 bytes | andypost |
#39 | field-test-type-2083803-39.patch | 27.23 KB | andypost |
#38 | interdiff.txt | 2.06 KB | andypost |
Comments
Comment #1
BerdirHere's a first patch.
Comment #3
andypostFiled separate issue to fix broken test #2087449: Fix broken tests FieldAttachOtherTest->testFieldAttachCache()
Fixed tests:
ShapeItemTest
,FieldValidationTest
Comment #4
andypostReverted back changed test about 'default_value_function' - that's needed
Removed hook_field_load - prepareCache() already there
Comment #5
andypostFixed FieldInfoTest
Comment #6
andypostAll tests are passed locally.
1)
FieldAttachOtherTest.php
changed according #2087449: Fix broken tests FieldAttachOtherTest->testFieldAttachCache()2)
field_test_widget_multiple_validate
moved to public static methodTestFieldWidgetMultiple::multipleValidate()
PS
drupal8.field-system.2083803-6-noinstallfile.patch
test the need in install fileComment #8
andypostA bit more clean-up
Comment #9
amateescu CreditAttribution: amateescu commentedWhy don't you get the field plugin definition like everyone else and duplicate all the data here? :)
Comment #10
andypostSuppose better to rename this to
expectedFieldInfo()
because this config used to validate result of
getDefinition()
Comment #11
BerdirI don't think we need to document how it was called in 7.x..
Can be written as return empty($this->value).
Comment #12
BerdirAbout method name, I'd suggest getExpectedFieldTypeDefinition() or something similar with getExpected..()
Comment #13
andypostaddressed
Comment #15
andypost#13: drupal8.field-system.2083803-13.patch queued for re-testing.
Comment #16
BerdirInterdiff looks fine to me, so +1 from my side. A bit surprised that we apparently don't have any tests remaining that required the memorize stuff for load/insert/update, but fine with me if that went out somewhere.
Wondering if we should add something else to the list of arguments we put in there, it used to contain the items too, so maybe add $this or $this->getValue() (easier to serialize) to it?
@yched?
Comment #17
andypostI'd prefer to keep conversion issues in the scope, so filed follow-up #2089369: Extend tests for field hooks
Comment #18
BerdirNot saying we need more tests.
I'm just saying that I *thought* we had more tests for this and I was confused to not see more test fails. But fine with me, pretty sure we have enough implicit test coverage of those methods. If anything, we should write a unit test for this kind of test when we can mock everything we need to at some point.
Comment #19
andypost#13: drupal8.field-system.2083803-13.patch queued for re-testing.
Comment #20
andypost@yched we needs your approval!
merge HEAD
Comment #21
Berdir#20: drupal8.field-system.2083803-20.patch queued for re-testing.
Comment #23
andypost#20: drupal8.field-system.2083803-20.patch queued for re-testing.
Comment #24
andypostpatch still applies
Comment #25
BerdirFYI: #2083785: Add support for determining which field properties are cacheable will rename the prepareCache() method.
Comment #26
Berdir#20: drupal8.field-system.2083803-20.patch queued for re-testing.
Comment #28
BerdirRe-roll.
Comment #29
swentel CreditAttribution: swentel commentedLooks good, another critical down if green!
Comment #31
BerdirThis should fix the test fails.
Comment #32
swentel CreditAttribution: swentel commentedNitpick, should probably be \TestFieldConstraint no ? Or do we do this different for constraint classes ?
Comment #33
BerdirFixed that. No, constraint classes are not that special :)
Comment #34
swentel CreditAttribution: swentel commentedGreat!
Comment #35
andypostI think the removed
hook_field_load
should not be referencedAlso there's no need in .install file
Comment #36
BerdirI explicitly didn't remove the .install file because it has this hunk in there, which (even if not necessary anymore, not sure), has nothing to do with this and shouldn't be removed together with it?
Comment #37
BerdirAlso not sure why the getValue() change is necessary? But then !$this->isEmpty() would make more sense I think and the test should probably reference the fully namespaced classname.
Comment #38
andypostFixed to full namespace.
Was added in #565480: TF #2: Multilingual field handling and no more needed because field translation is changed
Comment #39
andypostAnd missed
isEmpty()
Comment #40
Berdir@install stuff: Ok, it's quite possible that it's no longer used, but it's not related to this issue and it only removes 50% of it, there's still the actual hook implementation. I'd suggest to not touch that here :)
Comment #41
andypostok, added follow-up #2106501: Remove unused hook_install() from field_test module
Comment #42
swentel CreditAttribution: swentel commentedOk, let's get this in now, good catches andypost!
Comment #43
catchCommitted/pushed to 8.x, thanks!
Comment #44.0
(not verified) CreditAttribution: commentedUpdated issue summary.