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.
Problem/Motivation
EntityType::setStorageClass()
does not return anything.
Per EntityTypeInterface::setStorageClass()
, however, it should return $this
.
Proposed resolution
Add a return statement.
Remaining tasks
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#15 | interdiff.txt | 706 bytes | eporama |
#15 | 2758559-15.patch | 1.16 KB | eporama |
#9 | interdiff.txt | 940 bytes | eporama |
#9 | 2758559-9.patch | 1.2 KB | eporama |
#7 | interdiff-2758559-3-7.txt | 915 bytes | leolandotan |
Comments
Comment #2
kamalrajsahu21 CreditAttribution: kamalrajsahu21 at Intelliswift commentedComment #3
kamalrajsahu21 CreditAttribution: kamalrajsahu21 at Intelliswift commentedHere is the patch. Please review and commit.
Comment #4
kamalrajsahu21 CreditAttribution: kamalrajsahu21 at Intelliswift commentedComment #5
tstoecklerThat looks's perfect, thanks!
Can you add a quick test to
EntityTypeTest
? There's already atestGetStorageClass()
so maybe we can add a littletestSetStorageClass()
below it.Comment #6
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedI'll try to work on this.
Comment #7
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedHere I have added a simple test method for the `setStorageClass` method.
Hope everything is in order.
Comment #9
eporama CreditAttribution: eporama as a volunteer and at Acquia commentedThe issue here is that
setStorageClass
is now returning a full entityType object, not just the name of the storage class itself.Also, since handlers is a private method of entityType, you can't call it here. So we have to use
getStorageClass
(which isgetHanderClass('storage')
). However, getHandlerClass has a check to make sure the class is defined, we can't test with just concatenating "NewTest" on the name of the class because then getHandlerClass returns null.You can, however, set up the entity with a null storage class and then change it to the
$controller
class.Comment #11
eporama CreditAttribution: eporama as a volunteer and at Acquia commentedJust FYI, looks like this failure is not specifically in this new patch, but due to #2749955: Random fails in UpdatePathTestBase tests. Will hold off on retesting until that is resolved.
Comment #13
eporama CreditAttribution: eporama as a volunteer and at Acquia commentedRequeueing the test in #9 as the random failures appear to have been accounted for.
Comment #14
larowlanLooking good - comments follow
You need to test what you're actually fixing.
Comment #15
eporama CreditAttribution: eporama as a volunteer and at Acquia commentedRight. New test does check to make sure that what we're getting back is
$this
.Do we need any kind of testing that setStorageClass is actually setting a storage class? That was more what the original test was attempting, but not what this issue was fixing.
Comment #16
claudiu.cristeaThat seems to be covered by
EntityTypeTest::testGetStorageClass()
.Looks good to go.
Comment #18
xjmCommitted 24d528d and pushed to 8.3.x. Thanks!
Comment #19
tstoecklerThanks all, great to see this fixed!
I hadn't noticed this has been auto-moved to 8.3.x but as a bug fix, this should go into 8.2.x as well, so re-opening for that.
Comment #21
tstoecklerSent for a test run on 8.2.x now (and accidentally retested on 8.3.x, oops...)
Comment #23
tstoecklerComment #24
xjmWell, this changes the return value of the implementation (to match the interface), so it does have the teeniest bit of potential disruption.
However, it's very theoretical and unlikely that something could possibly rely on this implementation returning NULL, so I think it is fine to backport while we are still in beta. (I would not during RC or in a patch release, since
EntityType
is an important base class.)Thanks @tstoeckler!
Comment #27
rangana CreditAttribution: rangana at Promet Source commented