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.
config_test module used to test Configurables but still does not uses EntityFormController
entity_test module could be a good example of this conversion
Comment | File | Size | Author |
---|---|---|---|
#38 | config.test-form.38.patch | 28.8 KB | sun |
#38 | interdiff.txt | 2.38 KB | sun |
#33 | config.test-form.34.patch | 29.19 KB | sun |
#33 | interdiff.txt | 3.31 KB | sun |
#29 | config.test-form.29.patch | 26.15 KB | sun |
Comments
Comment #1
larowlanTagging
Comment #2
kbasarab CreditAttribution: kbasarab commentedExample of what happened in entity_test of this type of conversion is available at:
http://drupal.org/files/et-form_controller-1757232-17.patch
Comment #3
kbasarab CreditAttribution: kbasarab commentedThis should give at the least 3 fails. Posting it though because I'm not sure if I'm going at this the right way or not. First crack looking into these Entity Controllers.
I do keep getting this message:
"Declaration of Drupal\config_test\ConfigTestFormController::form() should be compatible with that of Drupal\Core\Entity\EntityFormControllerNG::form()"
Which I think is causing the page title test portions to fail.
But letting the bot hit it to get us started...
Comment #5
kbasarab CreditAttribution: kbasarab commentedStupid Mac files. Rerolled...
Comment #6
andypostplease revert this back
No reason, menu_item already provides one
Use $entity->label() here.
Not sure that t() needed here, this is a test module.
Controller should know entity fields also label() could return translated value...
Messages should contain label() not id()
Probably tests require some fixes
Comment #8
kbasarab CreditAttribution: kbasarab commentedThanks andypost. I'll take a look at those this weekend or early next week.
Comment #9
kbasarab CreditAttribution: kbasarab commentedFixed the items AndyPost was mentioning.
Though I didn't understand this one as the other items in this form have default_value's also:
Also not sure why my tests are failing on: "Declaration of Drupal\config_test\ConfigTestFormController::form() should be compatible with that of Drupal\Core\Entity\EntityFormControllerNG::form()"
If anyone could help guide me on that item and how to try and correct that would be awesome. The actual fails in the test results are 3 and they are for Drupal titles. When I try testing visually via module I get "Error" as my page title and the only thing I can think is from those warnings. Any help would be appreciated.
Comment #10
andypost@kbasarab take a look at patch provided by @tim.plunkett #1588422-133: Convert contact categories to configuration system
Any reason to have 2 menu callbacks?
This should be $entity->label;
EDIT It looks like most of this already in core
See core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test folder
Comment #11
tim.plunkettThe second is a MENU_DEFAULT_LOCAL_TASK, that's fine.
No it shouldn't :)
Comment #12
tim.plunkettComment #14
tim.plunkettHere's a bunch of fixes, they're mostly reverts.
Comment #16
kbasarab CreditAttribution: kbasarab commentedSo here is a patch that passes but I want to get clarification from someone on what I've done. The failures above were located within
core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.php
The portion I removed was there to "verify the appropriate module API hooks have been invoked."
Since this is now using EntityFormController if I'm not mistaken wouldn't these API hooks be tested during the EntityCrudHookTestCase?
Comment #17
kbasarab CreditAttribution: kbasarab commentedRemoves reference to hooks file and the require once in config_test.module to further remove the need for testing API hooks since that should now be done by EntityFormController and the EntityCrudHookTestCast.
Comment #18
tim.plunkettI agree, those shouldn't be needed here.
I don't see this being used, why bother adding it?
There should be a space after the php tag.
I don't see the reason to use EntityFormControllerNG instead of EntityFormController, its a compatibility layer for fieldable entities, which config_test isn't.
Comment #19
kbasarab CreditAttribution: kbasarab commentedhook_perm was implemented because of the example I was basing off of. Removed now.
Added space between opening php statement and @file block.
Changed EntityFormContrllerNG to use EntityFormController. Was originally added trying to debug the spelling error of interface.
Also found quite a few other references to the hook testing. Those have been removed as well.
Comment #20
sunComing from: #1588422: Convert contact categories to configuration system
I've no idea why you're deleting lots of required/essential test coverage here; I guess that must have been a bogus re-roll/merge or something.
Anyway, I'm working on this. Essential patch is done, but I'm adding some serious test coverage for ConfigEntity CRUD operations, so as to ensure that the form controller (and any other code) gets and sees the right properties and return values.
Comment #21
sunReverted bogus removal of config import support code and test coverage.
Added ConfigEntiy CRUD test coverage; fixed bugs.
Comment #22
andypostAwesome! Just a few questions:
Maybe better make it in Entity?
So getOriginalID() could be used when machine name is changed?
Not sure that usage of t() good for test module, but this makes sense as common pattern
Is there issue for that? Are you sure that process* function should detect entityForms?
Comment #23
sunNope, getOriginalID() only exists on ConfigEntityBase/ConfigEntityInterface. And so does setOriginalID().
During saving, yes. (i.e., methods and hooks being invoked could do that.) ConfigStorageController only updates the originalID as last step, after the config entity was saved.
That's a good point - so far, config_test.module does not use t() anywhere and so this page title shouldn't use t() either.
There was no issue for it yet, but now there is:
#1820834: Change #type machine_name default 'source' element to 'label'
Comment #24
sunReplaced t() with format_string().
Comment #26
andypostIt seems something in this tests is fragile, or introduces random failures
head broken in 4365f50..0b70f27
Comment #27
andypost#24: config.test-form.24.patch queued for re-testing.
Comment #29
sunMerged HEAD.
Comment #31
andypost@tim.plunkett - Suppose this change is not appropriate to views, please comment about
Comment #32
tim.plunkettThe views test assumes the following (where 'name' is currently our id_key):
That is, changing the ID and saving is a copy, not a rename. If that's not supposed to be the case, then the test needs to be updated (ViewStorageTest::saveTests())
Comment #33
sunSo I starred at this code for quite some while, and even had to patch Simpletest in order to figure out which exact operation in eh test is throwing that entity exception...
...only to realize that this entire test method does nothing else than duplicating tests for bare configurable/configuration system functionality, and has absolutely no business in doing so.
Tests that are duplicating test coverage are one of the worst offenders in testing. There are situations in which one cannot prevent it; e.g., implicitly testing the router system, redirect behavior, and other facilities that happen to be part of the test execution or its assertions. However, whenever a test or test method factually duplicates existing, dedicated test coverage elsewhere, that is a bug.
This is the case here. The test loads a Configurable, changes entity properties, saves it, recreates it from bare config, saves it under a different ID, and reloads it as entity. Nothing else is tested. This test method has absolutely nothing to do Views functionality.
Thus:
Removed off-topic test coverage from ViewStorageTest.
Added ConfigEntityBase::createDuplicate() override for extra safety.
Comment #34
tim.plunkettThose tests methods were present all through our port, even before ConfigEntities, just to ensure Views could do what we needed it to.
I'm definitely okay with removing it now.
And the createDuplicate() change looks good.
Pre-emptive RTBC if it's green.
Comment #35
sun#33: config.test-form.34.patch queued for re-testing.
Comment #36
sunThis is still blocking the follow-up fixes in #1588422: Convert contact categories to configuration system
Comment #37
catchThis no longer applies unfortunately. I found a couple of minor things but otherwise it looks good.
This could do with an explantion why we're explicitly checking for empty strings/NULL (as opposed to empty or similar).
That's the same check more or less. Also how can an ID be passed that's actually an empty string or NULL?
I actually ran into an issue with this on a Drupal 6 client site - there was code setting $node->nid = NULL when creating new nodes, and that resulted in deleting the node_acess record with nid = 0 or similar - but it was horribly broken code and throwing an exception there might have been friendlier.
Is there an issue open for the @todo?
Comment #38
sunBack to RTBC.
Comment #39
webchickI didn't review this overly thoroughly, but it looks like catch did already, and his feedback was addressed.
Committed and pushed to 8.x. Thanks!