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.
See ConfigContext::setUuid()
public function setUuid() {
$uuid = new Uuid();
$this->uuid = $uuid->generate();
}
We should inject dependencies, not hardcode em
Comment | File | Size | Author |
---|---|---|---|
#174 | drupal8.base-system.1969572-174.patch | 58.39 KB | neclimdul |
#174 | interdiff.txt | 2.67 KB | neclimdul |
#170 | drupal8.base-system.1969572-170.patch | 58.04 KB | neclimdul |
#170 | interdiff.txt | 1.19 KB | neclimdul |
#168 | drupal8.base-system.1969572-168.patch | 59.65 KB | neclimdul |
Comments
Comment #1
larowlanTagging
Comment #2
larowlanFirst cut
Comment #4
dawehnerSo this code could live in the CoreBundle instead.
Comment #5
larowlanAlso for tests we have $this->container no need for \Drupal::service('uuid')
Includes changes from #4 and a coding standards cleanup.
Comment #7
larowlanAdding changes notice tag, this removes the Uuid factory in favour of a service.
Comment #8
larowlanWe might need to handle registering the service in the installer....tipping this might fail
Comment #9
larowlanRegisters in the installer
Comment #11
larowlanshould be $uuid_class
But fixing that results in
Circular reference detected for service "config.factory", path: "config.factory -> config.context -> config.context.factory"
Comment #12
larowlanShould install now
Comment #14
larowlanThis time?
Installs locally
Comment #16
larowlanGoing to move the unit test to phpunit instead of UnitTestBase
Comment #17
larowlanMoves to php unit test
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedthis looks really good:D, thanks!
just a quick scan:
any reason it is uuidService and not just uuid?
Now that it is phpunit, no point having the Unit in the name..should be just UuidTest
Comment #19
dawehnerTechnically you wouldn't need to implement the interface as the parent class already does it, but it feels like a good way to document the dependency.
Maybe this is a really dumb question, but why do you need to mock something if you change anything / disable the constructor etc.
Maybe out of scope but shouldn't we also test the container compilation method? (that's maybe hard). If we would test the UUID classes directly by just creating it, for example the test would be easier to read. Then we could test all three UUID classes at the same time, if available.
Comment #20
larowlanMany of the entity classes etc use ->uuid as a property, wanted to keep the distinction between a uuid value and the uuid service.
Will fix
CoreBundle::registerUuid typehints the containerbuilder, we only care about the ->register() method but as we don't care about what it returns, the default mock behaviour (return NULL for all methods) is sufficient.
Also I'm using CoreBundle::registerUuid to get the class name because a) DRY - saves code duplication b) Doing this tests that logic
The point of ::registerUuid which was ::determinePlugin in the old factory is to get the best available implementation.
We can't test the windows (com) implementation without windows and the Pecl one is only around if Pecl is installed.
Doing it this way means we test the detection as part of the unit test.
Comment #21
larowlanRemoves Unit from test class as per #18
Comment #22
dawehnerI guess none of theme exist on the testbot? We could iterate over all of these classes and see whether they exist. If they exist create a class and test it.
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedclosed #2003762: Convert system module's Uuid unit tests to phpunit as dup
Comment #24
dawehnerLet's play around with the idea of #22
Comment #25
dawehnerI probably missed the test file.
Comment #27
dawehner#25: drupal-1969572-25.patch queued for re-testing.
Comment #29
dawehner#21: uuid-1969572.21.patch queued for re-testing.
Comment #30
damiankloip CreditAttribution: damiankloip commentedThe UuidBase class was missing, looking at the rest of the patch, it now only needs the validate() method in. I also removed the @todos added as we can now inject dependencies into entity controllers.
Comment #32
damiankloip CreditAttribution: damiankloip commentedPatch in #30 is good. I just stupidly attached the interdiff with a .patch extension..
Comment #33
ParisLiakos CreditAttribution: ParisLiakos commentedthis should be an array right? so i guess you forgot [] inside the conditional..
also i believe we can get rid of some use statements in .install files that used to do new Uuid();
Besides that this looks ready, thanks both:)
Comment #34
damiankloip CreditAttribution: damiankloip commentedGood points! Let's quickly change that and remove those.
Comment #35
ParisLiakos CreditAttribution: ParisLiakos commentedthank you!
Comment #36
dawehnerI'm sorry for this mistake!
Comment #37
BerdirI think this is now possible?
That said, #1777956: Provide a way to define default values for entity fields will move it into a Field for which injection is not yet possible and it will conflict with this.
Comment #38
alexpottNeeds a reroll
Comment #39
alexpottComment #40
damiankloip CreditAttribution: damiankloip commentedRerolled
Comment #42
damiankloip CreditAttribution: damiankloip commentedSorry field module.
Comment #43
ParisLiakos CreditAttribution: ParisLiakos commentedback to rtbc then:)
Comment #44
alexpottI get a local PHPUnit fail after applying the patch...
Comment #45
larowlanLets see how this goes.
Comment #46
dawehnerAs long we don't mock any methods we could also just use the object itself.
Comment #48
dawehner#46: uuid-1969572-46.patch queued for re-testing.
Comment #50
dawehnerThe fieldstoragecontroller had a constructor, so it had to be adapted as well.
Comment #52
damiankloip CreditAttribution: damiankloip commentedWe need to pass the uuid service into FieldStorageController, and implement the same for FieldInstanceStorageController.
Comment #53
dawehnerGood catch!
Comment #54
alexpottNeeds a reroll...
Comment #55
pwieck CreditAttribution: pwieck commentedWorking on re-roll
Comment #56
pwieck CreditAttribution: pwieck commentedreroll
Comment #58
pwieck CreditAttribution: pwieck commented#56: 1969572-56.patch queued for re-testing.
Comment #60
pwieck CreditAttribution: pwieck commented#56: 1969572-56.patch queued for re-testing.
Testbot is being 'testy' today.
Comment #62
pwieck CreditAttribution: pwieck commented#56: 1969572-56.patch queued for re-testing.
This shouldn't be failing
Comment #64
ParisLiakos CreditAttribution: ParisLiakos commented#56: 1969572-56.patch queued for re-testing.
Comment #65
pwieck CreditAttribution: pwieck commentedReady for review!
Comment #66
ParisLiakos CreditAttribution: ParisLiakos commentedback to rtbc
Comment #67
ParisLiakos CreditAttribution: ParisLiakos commentedactually, i just checked the test again...and found quite some issues..patch coming
Comment #68
ParisLiakos CreditAttribution: ParisLiakos commentedso, this will make sure to test the max available implementations.the elseif was not needed there..also i removed the unneeded mock of container builder...i read an article a few days back (if i find the link i ll post it)..we should mock when absolutely necessary not the other way around..this way we test more stuff and its not that makes phpunit any slower;)
lets set the good example in core
Comment #69
damiankloip CreditAttribution: damiankloip commentedNot sure if that is 'quite some issues' but those changes look great to me :)
I don't see why this wouldn't come back green.
Comment #70
ParisLiakos CreditAttribution: ParisLiakos commentedthanks, and you are right, language failure there..should be "a couple issues" :P
Comment #71
alexpottYAR...
Comment #72
pwieck CreditAttribution: pwieck commentedWorking on re-roll
Comment #73
pwieck CreditAttribution: pwieck commentedRe-rolled
Comment #74
pwieck CreditAttribution: pwieck commentedchanging status
Comment #76
pwieck CreditAttribution: pwieck commentedHere is the failing point. If someone can point out problem I will fix.
Comment #77
BerdirThat part changed quite a bit, you can completely drop that and also stop injecting that into the database storage. Instead, this is now generated in UuidItem.php, where you can't inject this, so instead you just have to call \Drupal::service('uuid') there.
Comment #78
hussainwebThe problem with the patch is an unclosed foreach loop in that file. I can't figure out where it applies exactly, and so I am trying out a reroll myself. :)
Comment #79
hussainwebHere is the rerolled patch.
Comment #81
pwieck CreditAttribution: pwieck commented#79: uuid-service-1969572-79.patch queued for re-testing.
Comment #83
hussainwebDoing a manual test now just to make sure.
Comment #84
BerdirYou can remove all this, it's no longer done here.
This is also an accidental revert, just revert all changes in EntityNG
Same, drop this :)
Instead of all that, the only place that now uses Uuid is UuidItem.php, see my comment above.
Comment #85
hussainweb@Berdir, what about the UuidInterface parameter in DatabaseStorageController.php. I see it is used here:
Should these changes be reverted too?
Comment #86
hussainwebBTW, I fixed the problem with previous failure. I am not sure how the original test passed without this (maybe it got added later). The problem was that the old Uuid class was referred to in
\Drupal\Core\Entity\Field\Type\UuidItem
which caused the error. I also found the usage in\Drupal\system\Tests\Entity\EntityFieldDefaultValueTest
and I fixed that too.I will post the patch once the changes in DatabaseStorageControllerNG.php are clarified.
Comment #87
BerdirThere is a single usage of that class left (being converted in #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest). After that happened, the DatabaseStorageController and DatabaseStorageControllerNG classes will be merged and we will no longer need this. But for now, keep that part.
Comment #88
hussainwebDo you mean should I keep the changes in DatabaseStorageControllerNG.php from the original patch in #73 too? I see notices after removing those changes because of this line in DatabaseStorageController.php in the constructor:
And I just can't remove that because uuidService is used later in the create() method.
Comment #89
hussainwebI am still a little confused about @Berdir's suggestions, so I am just attaching a patch without those, just to see the test results.
Comment #90
BerdirThe UuidItem class was added yesterday, that's why it passed before :)
Yes, sorry, you need to keep the constructor changes in the user and NG storage controllers so that you can pass the UUID service through. but the snippet in NG::create() can be removed.
Comment #91
hussainwebAttaching changes as per #90. Thanks!
Comment #93
hussainwebFixing a small typo...
Comment #94
neclimdulre-roll on top of #1893820: Manage entity field definitions in the entity manager
no changes.
Comment #96
neclimdulDid a review and resolved a lot of my complaints about dealing with the UUID system. The only real weakness seems to be the @todo in EntityNG and maybe an issue summary :)
Comment #97
neclimdulI take that back, just noticed the documentation for isValid is duplicated on the interface and the base implementation. I prefer this was just a method we could call instead of requiring a base implementation without any real utility but I don't know where it would live.
"Plugins should not implement validation..." is also weird for something on an interface because it literally requires you to implement it.
Comment #98
neclimdulPossible implementation. Also removed stray references to "plugins" which isn't really what's happening in this system. Just interface implementations.
Comment #100
neclimdul#98: uuid-service-1969572-98.patch queued for re-testing.
Comment #102
Berdir#98: uuid-service-1969572-98.patch queued for re-testing.
Comment #104
neclimdulAll tests pass locally using run-tests.sh. not really sure what's going on here since nothing functional should have changed since the last pass.
Comment #105
Berdir#98: uuid-service-1969572-98.patch queued for re-testing.
Comment #107
tim.plunkett#1777956: Provide a way to define default values for entity fields went in with the line:
Causing
Fatal error: Call to undefined method Drupal\Component\Uuid\Php::isValid() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldDefaultValueTest.php on line 57
Comment #108
neclimdulThanks! That wasn't on the branch I rebased before posting and ran the tests on. Re-rebased and fixed this call.
Comment #109
neclimdulwoops
Comment #110
dawehnerThis certainly need a rerole.
Comment #111
hussainwebI am attaching a reroll. Some points:
In
Drupal\user\Tests\Views\Argument\RolesRidTest
:The conflict was :
I mixed them to add $uuid before $entity_query_factory. I repeated similar steps across all conflicts.
Comment #113
neclimdullets try this. needed another re-roll anyways.
Comment #115
neclimduland another re-roll... config moved into plugins so a file moved.
Comment #116
neclimdulComment #118
neclimdullast fail was #1988508: Stop pretending we support Symfony bundles when we really just have service providers breaking install. Interdiff for that attached and rebased to fix another minor conflict in use statements not show.
really tired of chasing core...
Comment #120
neclimdulgit grep shows a bunch more 'new Uuid()' matches that must not have been there when the logic of this patch was written. We'll have to kill those before these tests will pass now. I'll be gone for the afternoon if someone wants to take this on.
Comment #121
BerdirThis should be all of them.
Comment #123
ParisLiakos CreditAttribution: ParisLiakos commentedComment #124
neclimdulI'm ok with RTBC'ing this but there really hasn't been any discussion in a while though because of the quick re-rolls. Its pretty straight forward so if someone else agrees lets bump it up.
Comment #125
neclimdulmerge conflict with #1810370: Entity Translation API improvements. no changes.
Comment #127
neclimdul#125: uuid-service-1969572-125.patch queued for re-testing.
Comment #128
neclimdulHEAD was broken #2024833: Adopt load() and loadMultiple() on entity storage controllers
Comment #129
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #130
neclimdulneed re-roll
Comment #131
hussainwebHere is a reroll attempt.
Comment #133
dawehnerFound a couple of more places, which does not need the Uuid use statement anymore and rerolled the patch.
Comment #135
xjmWe don't add the "Needs change notification" tag until the patch is committed. :)
Comment #136
neclimdulreroll around some things like #2046951: UUIDs have 16 bytes, not 32 and add some more stuff that's gone in. Noticed some Doc problems while re-rolling but didn't fix because it was later. Lets see how testing goes though.
Comment #138
dawehnerLet's fix this tiny kind of unimportant line.
Comment #139
neclimdulyeah... saw that and wrote the same patch and was about to upload it :)
Comment #141
neclimdulFix at least 2 of the test errors. The config error and the image test fatal error.
first interdiff is doc changes from before, second is bug fixes.
Comment #142
tim.plunkettOut of scope, but whatever
When its in a comment, please "UUID" (I didn't highlight every one, but there are a bunch)
This is not a storage controller, its the Entity class.
$entity_manger? Might as well leave it alone honestly...
Whoops!
Should leave this use statement in to simplify this line
Not doing yourself any favors here on rerolls...
Comment #143
neclimdulThe doc changes aren't really a big deal, its the moving UUID calls and additions that have really caused trouble.
Even so, they are far from the only cleanups those files need so I've taken them out.
Comment #145
Berdir#143: uuid-service-1969572-143.patch queued for re-testing.
Random missing table fail.
Comment #147
neclimdulBecause i'm lazy no interdiff but there where just 2 small config() -> Drupal::config() changes rebase handled.
Comment #148
dawehnerIt seems to be that the uuid could be injected into that class. The Drupal::service call thought would just be moved to ImageStyle.php but this still seems to be an advantage.
We could also switch to String::format
Theoretically all this foreach could be solved with dataProviders but I am not sure whether this concept if valid in this case.
isValid should already return a boolean value
Comment #149
tim.plunkettI'd prefer to keep the service calls in the plugin bags, they're not really set up for injection, and they're rather one-off. You would just replace the bag altogether if you needed.
Comment #150
ParisLiakos CreditAttribution: ParisLiakos commentedComment #151
star-szrTag fix, WSSCI -> WSCCI.
Comment #152
star-szrSorry for the noise, didn't check autocomplete.
Comment #153
neclimdulFinally another patch. Rerolled and then interdiff from there with the changes from #148 minus #149.
Comment #154
neclimdulRe-roll with "Drupal" to "\Drupal" changes.
Comment #156
neclimdul#154: drupal8.base-system.1969572-154.patch queued for re-testing.
Comment #158
neclimdulI can't recreate this... recreated the patch just to make sure.
Comment #159
dawehnerIt would be cool to have also one @group Drupal in order to be able to just run all of these tests.
Some short documentation lines could be added here.
Comment #160
jibranI think we should declare $instances array() first.
Comment #161
socketwench CreditAttribution: socketwench commentedRerolled, incorporated changes from #160.
Comment #162
neclimdulreroll around #1605290: Enable entity render caching with cache tag support(namespace conflict) and fixes from reviews.
Interdiff includes #161 because local branch, rebase blah blah I am lazy.
Comment #163
ParisLiakos CreditAttribution: ParisLiakos commentedso lets fix the phpunit test a bit. otherwise its ready to go
Comment #164
jibran#160 is still not fixed in last patch.
Comment #165
ParisLiakos CreditAttribution: ParisLiakos commentedyes, you are completely right. lets move this to the rest of our phpunit tests standards. way more to extend it now with addional array elements
Comment #166
jibranThank you @ParisLiakos for the changes. As #159 is addressed and as per #163 this is good to go so RTBC for me.
Comment #167
neclimdulThese changes removed the messages added to address #159.
I'll reroll.
this seems less clear then the FQ class name.
Comment #168
neclimdulActually the more I look at it the less useful the data-provider seems for test(Uuid)Validation. We're not re-using it and it obfuscates the test. Since you guys are sprinting out of sync with me here's a patch revering the changes which I think is the better approach.
Comment #170
neclimdulhunk removed thanks to #2095115: delete ViewTestConfigInstaller and have ViewTestData create views, not 'import' them
cleaned up a couple things i noticed while making sure rebase was sane.
Comment #171
neclimdullets run testbot like a pro.
Comment #172
ParisLiakos CreditAttribution: ParisLiakos commentedYou dont have to reuse a data provider..look across core, you will see most tests and providers are couples. ie one data provider for each test method. Providers are all about making your test cases easier extendable and cleaner. Dont you understand what the test actually does by just checking its docblock and its parameters?
Without dataproviders you have to read the whole method
PHPunit gives those info by default. Here how it looks when one data provider set fails:
Which is a lot better than concatenating strings and variables ;)
Comment #173
ParisLiakos CreditAttribution: ParisLiakos commentedsigh
Comment #174
neclimdulI thought it was surprising that tests would use a dataprovider and assert on a bool argument instead of using the explicit checks but after reviewing a ton of dataProviders it does seem to be fairly common.
I did notice that most of them seem to take a failure message and that seems a fair middle ground because the default doesn't really explain what failed. The original comments weren't that clear either really so I cleaned them up a bit.
One additional change from #165 is using assertSame instead of assertEqual which will ensure we're returning equivalent bools instead of asserting loosely typed thruthyness. This also seemed to be the norm and is in fact what we wanted. Noted since it doesn't show in the interdiff.
Comment #175
ParisLiakos CreditAttribution: ParisLiakos commentedagreed
Comment #176
webchickCommitted and pushed to 8.x. Thanks!
This'll need a change notice.
Comment #177
neclimdulDid a quick search, these are likely to need to be updated:
https://drupal.org/list-changes?keywords_description=uuid&to_branch=8.x
Doesn't look like the UUID system ever got a change notice to be updated or referenced. #1252486-111: Low level UUID API in core
Comment #178
Gábor HojtsyAdded a change notice at https://drupal.org/node/2184281 which covers this one and the original issue to add UUIDs.