Closed (fixed)
Project:
Typed Data API enhancements
Version:
8.x-1.x-dev
Component:
Miscellaneous
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
2 Feb 2020 at 17:05 UTC
Updated:
1 Jan 2023 at 11:04 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
jonathan1055 commentedUnpostponing now that #3109923: Deprecation: add $defaultTheme - create FormWidgetBrowserTestBase class is done
Comment #3
jonathan1055 commentedand I forgot the vital change
Comment #4
jonathan1055 commentedChecking back with the problems we found on #3109923: Deprecation: add $defaultTheme - create FormWidgetBrowserTestBase class, in summary
Declaration of Drupal\Tests\typed_data\Functional\TypedDataFormWidget\FormWidgetBrowserTestBase::setUp($widgetId) should be compatible with Drupal\Tests\BrowserTestBase::setUp()which means we have to have a default value for $widgetIdDeclaration of Drupal\Tests\typed_data\Functional\TypedDataFormWidget\DatetimeWidgetTest::setUp(): void should be compatible with Drupal\Tests\typed_data\Functional\TypedDataFormWidget\FormWidgetBrowserTestBase::setUp($widgetId = ''): voidThis actually runs fine, but we get a coding standard failure for each one, stating:
I moved the createUser and login into this function too, just for convenience, as we do not now have a setUp() in FormWidgetBrowserTestBase. But if you would prefer, I can create that setUp and move the single line login into it.
Comment #6
jonathan1055 commentedNeeds Review. When this is done I can work on
#3109906: Textarea example form is limited to 8 character input
#3110036: Create assertIsApplicable() and assertIsNotApplicable() in widget base class
#3169256: Add test for DateTimeRange widget
Comment #7
tr commentedThis moves the createUser() into the Base setUp() and eliminates the widgetId property which isn't really needed.
Comment #8
jonathan1055 commentedJust a question, if we are working on this together then we need to agree how. I created a MR but you have provided a patch without interdiff. If you want to work with patches that ok, I just need to know.
Comment #9
jonathan1055 commentedYour changes look good. For the record, I have uploaded that interdiff. Yes that
widgetIdproperty is not needed. It was a remnant from my previous work on the preceding issue #3109923: Deprecation: add $defaultTheme - create FormWidgetBrowserTestBase class. Also moving the createUser() into a new base setUp() is much better than having it in the createWidget(). I had wondered about doing that myself.I spotted one unnecessary 'drupalLogin(...createUser()' which I will remove in the next patch.
Comment #10
jonathan1055 commentedPatch to remove the unnecessary login & createUser. When this passes I am happy for this to be RTBC if you are.
Comment #11
tr commentedThe MR needs to be rebased and the UI gives me an error message when I try, so back to the patch ...
I made a spelling change to what you have in #10, but otherwise this is the same. I just want to re-test to be sure before I commit.
Comment #13
tr commentedCommitted.
Comment #15
jonathan1055 commentedThe message above was automatically duplicated from the MR.
Thanks for the commit. I have unpostponed #3110036: Create assertIsApplicable() and assertIsNotApplicable() in widget base class