See ConfigContext::setUuid()

  public function setUuid() {
    $uuid = new Uuid();
    $this->uuid = $uuid->generate();
  }

We should inject dependencies, not hardcode em

CommentFileSizeAuthor
#174 drupal8.base-system.1969572-174.patch58.39 KBneclimdul
#174 interdiff.txt2.67 KBneclimdul
#170 drupal8.base-system.1969572-170.patch58.04 KBneclimdul
#170 interdiff.txt1.19 KBneclimdul
#168 drupal8.base-system.1969572-168.patch59.65 KBneclimdul
#168 interdiff.txt2.62 KBneclimdul
#165 drupal-uuid_as_service-165.patch59.85 KBParisLiakos
#165 interdiff.txt602 bytesParisLiakos
#163 drupal-uuid_as_service-163.patch59.82 KBParisLiakos
#163 interdiff.txt4.04 KBParisLiakos
#162 drupal8.base-system.1969572-162.patch59.61 KBneclimdul
#162 interdiff.txt2.31 KBneclimdul
#161 drupal8.base-system.1969572-161.patch58.98 KBsocketwench
#158 drupal8.base-system.1969572-158.patch59.16 KBneclimdul
#154 drupal8.base-system.1969572-154.patch59.63 KBneclimdul
#153 drupal8.base-system.1969572-153.patch59.12 KBneclimdul
#153 interdiff.txt6.1 KBneclimdul
#147 uuid-service-1969572-147.patch60.27 KBneclimdul
#143 interdiff.txt14.73 KBneclimdul
#143 uuid-service-1969572-143.patch60.26 KBneclimdul
#141 uuid-service-1969572-141.patch61.41 KBneclimdul
#141 interdiff.txt4.21 KBneclimdul
#141 interdiff2.txt6.17 KBneclimdul
#138 uuid-1969572-138.patch56.74 KBdawehner
#138 interdiff.txt614 bytesdawehner
#136 uuid-service-1969572-136.patch56.37 KBneclimdul
#133 uid-1969572-131.patch56.19 KBdawehner
#131 uuid-service-1969572-131.patch54.53 KBhussainweb
#125 uuid-service-1969572-125.patch55.89 KBneclimdul
#123 uuid-uuid-service-1969572-123.patch55.88 KBParisLiakos
#123 interdiff.txt3.1 KBParisLiakos
#121 uuid-uuid-service-1969572-121.patch55.99 KBBerdir
#121 uuid-uuid-service-1969572-121-interdiff.txt3.02 KBBerdir
#118 interdiff.txt1.5 KBneclimdul
#118 uuid-service-1969572-118.patch53.48 KBneclimdul
#115 uuid-service-1969572-115.patch53.44 KBneclimdul
#113 uuid-service-1969572-113.patch53.35 KBneclimdul
#111 uuid-service-1969572-111.patch52.9 KBhussainweb
#108 uuid-service-1969572-109.patch52.34 KBneclimdul
#98 uuid-service-1969572-98.patch51.56 KBneclimdul
#98 interdiff.txt4.89 KBneclimdul
#94 uuid-service-1969572-94.patch52.73 KBneclimdul
#93 uuid-service-1969572-93.patch52.76 KBhussainweb
#91 uuid-service-1969572-91.patch52.76 KBhussainweb
#89 uuid-service-1969572-89.patch53.21 KBhussainweb
#79 uuid-service-1969572-79.patch51.79 KBhussainweb
#73 1969572-73.patch51.67 KBpwieck
#68 1969572-68.patch51.85 KBParisLiakos
#68 interdiff.txt1.83 KBParisLiakos
#56 1969572-56.patch51.76 KBpwieck
#52 1969572-52.patch51.92 KBdamiankloip
#52 interdiff-1969572-52.txt4 KBdamiankloip
#50 uuid-1969572-50.patch49.24 KBdawehner
#50 interdiff.txt1.61 KBdawehner
#46 uuid-1969572-46.patch47.36 KBdawehner
#46 interdiff.txt1.45 KBdawehner
#45 uuid-1969572.45.interdiff.txt1.19 KBlarowlan
#45 uuid-1969572.45.patch47.14 KBlarowlan
#42 1969572-42.patch45.93 KBdamiankloip
#42 interdiff-1969572-42.txt742 bytesdamiankloip
#40 1969572-40.patch45.39 KBdamiankloip
#34 1969572-34.patch45.1 KBdamiankloip
#34 interdiff-1969572-34.txt6.8 KBdamiankloip
#30 1969572-31.patch42.28 KBdamiankloip
#30 interdiff-1969572-31.patch9.88 KBdamiankloip
#25 drupal-1969572-25.patch34.21 KBdawehner
#24 drupal-1969572-24.patch31.17 KBdawehner
#24 interdiff.txt5.7 KBdawehner
#21 uuid-1969572.21.interdiff.txt691 byteslarowlan
#21 uuid-1969572.21.patch34.18 KBlarowlan
#17 uuid-1969572.17.interdiff.txt0 byteslarowlan
#17 uuid-1969572.17.patch34.19 KBlarowlan
#14 uuid-1969572.14.interdiff.txt4.91 KBlarowlan
#14 uuid-1969572.14.patch29.57 KBlarowlan
#12 uuid-1969572.12.interdiff.txt1.18 KBlarowlan
#12 uuid-1969572.12.patch24.66 KBlarowlan
#9 uuid-1969572.8.interdiff.txt1.15 KBlarowlan
#9 uuid-1969572.8.patch24.1 KBlarowlan
#5 uuid-1969572.5.interdiff.txt9.17 KBlarowlan
#5 uuid-1969572.5.patch23.61 KBlarowlan
#2 uuid-1969572.2.patch16.2 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Tagging

larowlan’s picture

Status: Active » Needs review
FileSize
16.2 KB

First cut

Status: Needs review » Needs work

The last submitted patch, uuid-1969572.2.patch, failed testing.

dawehner’s picture

/**
* Determines the optimal implementation to use for generating UUIDs.
*
* The selection is made based on the enabled PHP extensions with the
* most performant available option chosen.
*
* @return string
* The class name for the optimal UUID generator.
*/
protected function determinePlugin() {
static $plugin;
if (!empty($plugin)) {
return $plugin;
}

$plugin = 'Drupal\Component\Uuid\Php';

// Debian/Ubuntu uses the (broken) OSSP extension as their UUID
// implementation. The OSSP implementation is not compatible with the
// PECL functions.
if (function_exists('uuid_create') && !function_exists('uuid_make')) {
$plugin = 'Drupal\Component\Uuid\Pecl';
}
// Try to use the COM implementation for Windows users.
elseif (function_exists('com_create_guid')) {
$plugin = 'Drupal\Component\Uuid\Com';
}
return $plugin;
}

So this code could live in the CoreBundle instead.

larowlan’s picture

Status: Needs work » Needs review
FileSize
23.61 KB
9.17 KB

Also for tests we have $this->container no need for \Drupal::service('uuid')
Includes changes from #4 and a coding standards cleanup.

Status: Needs review » Needs work

The last submitted patch, uuid-1969572.5.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record

Adding changes notice tag, this removes the Uuid factory in favour of a service.

larowlan’s picture

We might need to handle registering the service in the installer....tipping this might fail

larowlan’s picture

Registers in the installer

Status: Needs review » Needs work

The last submitted patch, uuid-1969572.8.patch, failed testing.

larowlan’s picture

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -116,4 +117,27 @@ public static function registerTwig(ContainerBuilder $container) {
+    $container->register('uuid', $uid_class);

should be $uuid_class

But fixing that results in Circular reference detected for service "config.factory", path: "config.factory -> config.context -> config.context.factory"

larowlan’s picture

Status: Needs work » Needs review
FileSize
24.66 KB
1.18 KB

Should install now

Status: Needs review » Needs work

The last submitted patch, uuid-1969572.12.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
29.57 KB
4.91 KB

This time?
Installs locally

Status: Needs review » Needs work

The last submitted patch, uuid-1969572.14.patch, failed testing.

larowlan’s picture

Going to move the unit test to phpunit instead of UnitTestBase

larowlan’s picture

Status: Needs work » Needs review
FileSize
34.19 KB
0 bytes

Moves to php unit test

ParisLiakos’s picture

Issue tags: +PHPUnit

this looks really good:D, thanks!
just a quick scan:

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.phpundefined
@@ -108,18 +109,21 @@ class ConfigImporter {
+    $this->uuidService = $uuid;

+++ b/core/lib/Drupal/Core/Config/Context/ConfigContext.phpundefined
@@ -50,13 +50,23 @@ class ConfigContext implements ContextInterface {
+  protected $uuidService;
...
+    $this->uuidService = $uuid;

@@ -89,8 +99,7 @@ public function set($key, $value) {
+    $this->uuid = $this->uuidService->generate();

+++ b/core/lib/Drupal/Core/Config/Context/ConfigContextFactory.phpundefined
@@ -28,13 +29,23 @@ class ConfigContextFactory {
+  protected $uuidService;

any reason it is uuidService and not just uuid?

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewTestData.phpundefined
--- /dev/null
+++ b/core/tests/Drupal/Tests/Component/Uuid/UuidUnitTest.phpundefined

+++ b/core/tests/Drupal/Tests/Component/Uuid/UuidUnitTest.phpundefined
@@ -0,0 +1,80 @@
+ * Contains \Drupal\Tests\Component\Uuid\UuidUnitTest.

Now that it is phpunit, no point having the Unit in the name..should be just UuidTest

dawehner’s picture

+++ b/core/lib/Drupal/Component/Uuid/Pecl.phpundefined
@@ -10,12 +10,13 @@
+class Pecl extends UuidBase implements UuidInterface {

+++ b/core/lib/Drupal/Component/Uuid/Php.phpundefined
@@ -16,10 +16,10 @@
+class Php extends UuidBase implements UuidInterface {

Technically 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.

+++ b/core/tests/Drupal/Tests/Component/Uuid/UuidUnitTest.phpundefined
@@ -0,0 +1,80 @@
+    $container = $this->getMockBuilder('Symfony\Component\DependencyInjection\ContainerBuilder')
+      ->getMock();

Maybe this is a really dumb question, but why do you need to mock something if you change anything / disable the constructor etc.

+++ b/core/tests/Drupal/Tests/Component/Uuid/UuidUnitTest.phpundefined
@@ -0,0 +1,80 @@
+    $class = CoreBundle::registerUuid($container);
+    $this->uuid = new $class();

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.

larowlan’s picture

any reason it is uuidService and not just uuid?

Many of the entity classes etc use ->uuid as a property, wanted to keep the distinction between a uuid value and the uuid service.

Now that it is phpunit, no point having the Unit in the name..should be just UuidTest

Will fix

Maybe this is a really dumb question, but why do you need to mock something if you change anything / disable the constructor etc.

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

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.

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.

larowlan’s picture

Removes Unit from test class as per #18

dawehner’s picture

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.

I 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.

ParisLiakos’s picture

dawehner’s picture

FileSize
5.7 KB
31.17 KB

Let's play around with the idea of #22

dawehner’s picture

FileSize
34.21 KB

I probably missed the test file.

Status: Needs review » Needs work
Issue tags: -Entity system, -PHPUnit, -Needs change record, -WSSCI Conversion

The last submitted patch, drupal-1969572-25.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#25: drupal-1969572-25.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1969572-25.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +PHPUnit, +Needs change record, +WSSCI Conversion

#21: uuid-1969572.21.patch queued for re-testing.

damiankloip’s picture

The 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.

Status: Needs review » Needs work

The last submitted patch, interdiff-1969572-31.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

Patch in #30 is good. I just stupidly attached the interdiff with a .patch extension..

ParisLiakos’s picture

+++ b/core/tests/Drupal/Tests/Component/Uuid/UuidTest.phpundefined
@@ -0,0 +1,101 @@
+    $this->uuidInstances[] = new $class();
...
+      $this->uuidInstances = new \Drupal\Component\Uuid\Pecl();
...
+      $this->uuidInstances = new \Drupal\Component\Uuid\Com();
...
+    foreach ($this->uuidInstances as $instance) {

this 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:)

damiankloip’s picture

Good points! Let's quickly change that and remove those.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thank you!

dawehner’s picture

I'm sorry for this mistake!

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -353,8 +352,9 @@ public function createDuplicate() {
+      // @todo Inject the uuid service into the storage controller once
+      //    possible.
+      $duplicate->{$entity_info['entity_keys']['uuid']} = \Drupal::service('uuid')->generate();

I 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.

alexpott’s picture

Issue tags: +Needs reroll

Needs a reroll

curl https://drupal.org/files/1969572-34.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 46181  100 46181    0     0  20914      0  0:00:02  0:00:02 --:--:-- 29284
error: patch failed: core/includes/install.core.inc:410
error: core/includes/install.core.inc: patch does not apply
error: patch failed: core/includes/update.inc:13
error: core/includes/update.inc: patch does not apply
error: patch failed: core/lib/Drupal/Core/Entity/Entity.php:7
error: core/lib/Drupal/Core/Entity/Entity.php: patch does not apply
error: patch failed: core/modules/config/lib/Drupal/config/Tests/ConfigImporterTest.php:58
error: core/modules/config/lib/Drupal/config/Tests/ConfigImporterTest.php: patch does not apply
error: patch failed: core/modules/contact/contact.install:54
error: core/modules/contact/contact.install: patch does not apply
error: patch failed: core/modules/field/field.install:389
error: core/modules/field/field.install: patch does not apply
error: patch failed: core/modules/user/lib/Drupal/user/UserStorageController.php:12
error: core/modules/user/lib/Drupal/user/UserStorageController.php: patch does not apply
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
damiankloip’s picture

Status: Needs work » Needs review
FileSize
45.39 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, 1969572-40.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
742 bytes
45.93 KB

Sorry field module.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc then:)

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I get a local PHPUnit fail after applying the patch...

1) Drupal\Tests\Component\Uuid\UuidTest::testGenerateUuid
Declaration of Mock_ContainerBuilder_f0586b86::addResource() should be compatible with that of Symfony\Component\DependencyInjection\ContainerBuilder::addResource()

/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Component/Uuid/UuidTest.php:41
/usr/lib/php/pear/PHPUnit/TextUI/Command.php:176
/usr/lib/php/pear/PHPUnit/TextUI/Command.php:129
larowlan’s picture

Lets see how this goes.

dawehner’s picture

FileSize
1.45 KB
47.36 KB

As long we don't mock any methods we could also just use the object itself.

Status: Needs review » Needs work
Issue tags: -Entity system, -PHPUnit, -Needs change record, -WSSCI Conversion

The last submitted patch, uuid-1969572-46.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#46: uuid-1969572-46.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Entity system, +PHPUnit, +Needs change record, +WSSCI Conversion

The last submitted patch, uuid-1969572-46.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.61 KB
49.24 KB

The fieldstoragecontroller had a constructor, so it had to be adapted as well.

Status: Needs review » Needs work

The last submitted patch, uuid-1969572-50.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4 KB
51.92 KB

We need to pass the uuid service into FieldStorageController, and implement the same for FieldInstanceStorageController.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Good catch!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll...

curl https://drupal.org/files/1969572-52.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 53162  100 53162    0     0  34761      0  0:00:01  0:00:01 --:--:-- 40213
error: patch failed: core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php:289
error: core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php: patch does not apply
error: patch failed: core/lib/Drupal/Core/Entity/DatabaseStorageController.php:376
error: core/lib/Drupal/Core/Entity/DatabaseStorageController.php: patch does not apply
error: patch failed: core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php:115
error: core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php: patch does not apply
error: patch failed: core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php:11
error: core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php: patch does not apply
error: core/modules/user/tests/Drupal/Tests/user/Views/Argument/RolesRidTest.php: No such file or directory
pwieck’s picture

Working on re-roll

pwieck’s picture

Status: Needs work » Needs review
FileSize
51.76 KB

reroll

Status: Needs review » Needs work
Issue tags: -Entity system, -PHPUnit, -Needs change record, -Needs reroll, -WSSCI Conversion

The last submitted patch, 1969572-56.patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review

#56: 1969572-56.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1969572-56.patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review

#56: 1969572-56.patch queued for re-testing.

Testbot is being 'testy' today.

Status: Needs review » Needs work

The last submitted patch, 1969572-56.patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review

#56: 1969572-56.patch queued for re-testing.

This shouldn't be failing

Status: Needs review » Needs work

The last submitted patch, 1969572-56.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +PHPUnit, +Needs change record, +Needs reroll, +WSSCI Conversion

#56: 1969572-56.patch queued for re-testing.

pwieck’s picture

Ready for review!

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

ParisLiakos’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs reroll

actually, i just checked the test again...and found quite some issues..patch coming

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.83 KB
51.85 KB

so, 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

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Not 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.

ParisLiakos’s picture

thanks, and you are right, language failure there..should be "a couple issues" :P

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

YAR...

curl https://drupal.org/files/1969572-68.patch  | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 53092  100 53092    0     0  29392      0  0:00:01  0:00:01 --:--:-- 33161
error: patch failed: core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php:118
error: core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php: patch does not apply
error: patch failed: core/lib/Drupal/Core/Entity/EntityNG.php:9
error: core/lib/Drupal/Core/Entity/EntityNG.php: patch does not apply
error: patch failed: core/modules/system/lib/Drupal/system/Tests/Uuid/UuidUnitTest.php:1
error: core/modules/system/lib/Drupal/system/Tests/Uuid/UuidUnitTest.php: patch does not apply
error: patch failed: core/modules/taxonomy/taxonomy.install:5
error: core/modules/taxonomy/taxonomy.install: patch does not apply
pwieck’s picture

Assigned: Unassigned » pwieck

Working on re-roll

pwieck’s picture

Assigned: pwieck » Unassigned
FileSize
51.67 KB

Re-rolled

pwieck’s picture

Status: Needs work » Needs review

changing status

Status: Needs review » Needs work

The last submitted patch, 1969572-73.patch, failed testing.

pwieck’s picture

Here is the failing point. If someone can point out problem I will fix.

diff --git a/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
index 73c903d..3269a9f 100644
--- a/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -9,13 +9,12 @@
 
 use Drupal\Core\Language\Language;
 use PDO;
-
 use Drupal\Core\Entity\Query\QueryInterface;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\DatabaseStorageController;
 use Drupal\Core\Entity\EntityStorageException;
 use Drupal\Core\TypedData\ComplexDataInterface;
-use Drupal\Component\Uuid\Uuid;
+use Drupal\Component\Uuid\UuidInterface;
 use Drupal\Core\Database\Connection;
 
 /**
@@ -54,8 +53,8 @@ class DatabaseStorageControllerNG extends DatabaseStorageController {
   /**
    * Overrides DatabaseStorageController::__construct().
    */
-  public function __construct($entity_type, array $entity_info, Connection $database) {
-    parent::__construct($entity_type,$entity_info, $database);
+  public function __construct($entity_type, array $entity_info, Connection $database, UuidInterface $uuid_service) {
+    parent::__construct($entity_type,$entity_info, $database, $uuid_service);
     $this->bundleKey = !empty($this->entityInfo['entity_keys']['bundle']) ? $this->entityInfo['entity_keys']['bundle'] : FALSE;
     $this->entityClass = $this->entityInfo['class'];
 
@@ -124,6 +123,9 @@ public function create(array $values) {
     // Set any passed values for non-defined fields also.
     foreach ($values as $name => $value) {
       $entity->$name = $value;
+    // Assign a new UUID if there is none yet.
+    if ($this->uuidKey && !isset($entity->{$this->uuidKey}->value)) {
+      $entity->{$this->uuidKey} =  $this->uuidService->generate();
     }
     $entity->postCreate($this);
Berdir’s picture

That 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.

hussainweb’s picture

Assigned: Unassigned » hussainweb

The 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. :)

hussainweb’s picture

Status: Needs work » Needs review
FileSize
51.79 KB

Here is the rerolled patch.

Status: Needs review » Needs work
Issue tags: -Entity system, -PHPUnit, -Needs change record, -Needs reroll, -WSSCI Conversion

The last submitted patch, uuid-service-1969572-79.patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review

#79: uuid-service-1969572-79.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Entity system, +PHPUnit, +Needs change record, +Needs reroll, +WSSCI Conversion

The last submitted patch, uuid-service-1969572-79.patch, failed testing.

hussainweb’s picture

Doing a manual test now just to make sure.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -15,7 +15,7 @@
-use Drupal\Component\Uuid\Uuid;
+use Drupal\Component\Uuid\UuidInterface;

@@ -54,8 +54,8 @@ class DatabaseStorageControllerNG extends DatabaseStorageController {
-  public function __construct($entity_type, array $entity_info, Connection $database) {
-    parent::__construct($entity_type,$entity_info, $database);
+  public function __construct($entity_type, array $entity_info, Connection $database, UuidInterface $uuid_service) {
+    parent::__construct($entity_type,$entity_info, $database, $uuid_service);

@@ -125,6 +125,11 @@ public function create(array $values) {
+    // Assign a new UUID if there is none yet.
+    if ($this->uuidKey && !isset($entity->{$this->uuidKey}->value)) {
+      $entity->{$this->uuidKey} =  $this->uuidService->generate();

You can remove all this, it's no longer done here.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -41,6 +41,7 @@ class EntityNG extends Entity {
+   * @todo: Provide a better way for defining default values.

@@ -545,7 +546,9 @@ public function createDuplicate() {
-      $duplicate->{$entity_info['entity_keys']['uuid']}->applyDefaultValue();
+      // @todo Inject the uuid service into the storage controller once
+      //    possible.
+      $duplicate->{$entity_info['entity_keys']['uuid']}->value = \Drupal::service('uuid')->generate();

This is also an accidental revert, just revert all changes in EntityNG

+++ b/core/modules/user/lib/Drupal/user/UserStorageController.phpundefined
@@ -11,6 +11,7 @@
+use Drupal\Component\Uuid\UuidInterface;

@@ -51,8 +52,8 @@ class UserStorageController extends DatabaseStorageControllerNG implements UserS
    */
-  public function __construct($entity_type, $entity_info, Connection $database, PasswordInterface $password, UserDataInterface $user_data) {
-    parent::__construct($entity_type, $entity_info, $database);
+  public function __construct($entity_type, $entity_info, Connection $database, UuidInterface $uuid_service, PasswordInterface $password, UserDataInterface $user_data) {
+    parent::__construct($entity_type, $entity_info, $database, $uuid_service);

@@ -66,6 +67,7 @@ public static function createInstance(ContainerInterface $container, $entity_typ
+      $container->get('uuid'),

Same, drop this :)

Instead of all that, the only place that now uses Uuid is UuidItem.php, see my comment above.

hussainweb’s picture

@Berdir, what about the UuidInterface parameter in DatabaseStorageController.php. I see it is used here:

     // Assign a new UUID if there is none yet.
     if ($this->uuidKey && !isset($entity->{$this->uuidKey})) {
-      $uuid = new Uuid();
-      $entity->{$this->uuidKey} = $uuid->generate();
+      $entity->{$this->uuidKey} = $this->uuidService->generate();
     }
     $entity->postCreate($this);

Should these changes be reverted too?

hussainweb’s picture

BTW, 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.

Berdir’s picture

There 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.

hussainweb’s picture

Do 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:

    $this->uuidService = $uuid_service;

And I just can't remove that because uuidService is used later in the create() method.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
53.21 KB

I am still a little confused about @Berdir's suggestions, so I am just attaching a patch without those, just to see the test results.

Berdir’s picture

Status: Needs review » Needs work

The 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.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
52.76 KB

Attaching changes as per #90. Thanks!

Status: Needs review » Needs work

The last submitted patch, uuid-service-1969572-91.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
52.76 KB

Fixing a small typo...

neclimdul’s picture

Status: Needs review » Needs work

The last submitted patch, uuid-service-1969572-94.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review

Did 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 :)

neclimdul’s picture

I 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.

neclimdul’s picture

Possible implementation. Also removed stray references to "plugins" which isn't really what's happening in this system. Just interface implementations.

Status: Needs review » Needs work
Issue tags: -Entity system, -PHPUnit, -Needs change record, -WSSCI Conversion

The last submitted patch, uuid-service-1969572-98.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review

#98: uuid-service-1969572-98.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, uuid-service-1969572-98.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#98: uuid-service-1969572-98.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Entity system, +PHPUnit, +Needs change record, +WSSCI Conversion

The last submitted patch, uuid-service-1969572-98.patch, failed testing.

neclimdul’s picture

All 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.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Entity system, -PHPUnit, -Needs change record, -WSSCI Conversion

#98: uuid-service-1969572-98.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Entity system, +PHPUnit, +Needs change record, +WSSCI Conversion

The last submitted patch, uuid-service-1969572-98.patch, failed testing.

tim.plunkett’s picture

#1777956: Provide a way to define default values for entity fields went in with the line:

$this->assertTrue($this->uuid->isValid($entity->uuid->value), format_string('%entity_type: Default UUID', array('%entity_type' => $entity_type)));

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

neclimdul’s picture

Thanks! That wasn't on the branch I rebased before posting and ran the tests on. Re-rebased and fixed this call.

neclimdul’s picture

Status: Needs work » Needs review

woops

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

This certainly need a rerole.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
52.9 KB

I am attaching a reroll. Some points:

In Drupal\user\Tests\Views\Argument\RolesRidTest:

The conflict was :

<<<<<<< HEAD
    $entity_query_factory = $this->getMockBuilder('Drupal\Core\Entity\Query\QueryFactory')
      ->disableOriginalConstructor()
      ->getMock();
=======
    $uuid = new Php();
>>>>>>> UUID changes

    // Creates a stub role storage controller and replace the attachLoad()
    // method with an empty version, because attachLoad() calls
    // module_implements().
<<<<<<< HEAD
    $role_storage_controller = $this->getMock('Drupal\user\RoleStorageController', array('attachLoad'), array('user_role', static::$entityInfo, $config_factory, $config_storage, $entity_query_factory));
=======
    $role_storage_controller = $this->getMock('Drupal\user\RoleStorageController', array('attachLoad'), array('user_role', static::$entityInfo, $config_factory, $config_storage, $uuid));
>>>>>>> UUID changes

I mixed them to add $uuid before $entity_query_factory. I repeated similar steps across all conflicts.

Status: Needs review » Needs work

The last submitted patch, uuid-service-1969572-111.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
53.35 KB

lets try this. needed another re-roll anyways.

Status: Needs review » Needs work

The last submitted patch, uuid-service-1969572-113.patch, failed testing.

neclimdul’s picture

and another re-roll... config moved into plugins so a file moved.

neclimdul’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, uuid-service-1969572-115.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
53.48 KB
1.5 KB

last 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...

Status: Needs review » Needs work

The last submitted patch, uuid-service-1969572-118.patch, failed testing.

neclimdul’s picture

git 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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.02 KB
55.99 KB

This should be all of them.

Status: Needs review » Needs work

The last submitted patch, uuid-uuid-service-1969572-121.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
3.1 KB
55.88 KB
neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

I'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.

neclimdul’s picture

merge conflict with #1810370: Entity Translation API improvements. no changes.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Entity system, -PHPUnit, -Needs change record, -Needs reroll, -WSSCI Conversion

The last submitted patch, uuid-service-1969572-125.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +PHPUnit, +Needs change record, +Needs reroll, +WSSCI Conversion

#125: uuid-service-1969572-125.patch queued for re-testing.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community
YesCT’s picture

Issue tags: -Needs reroll +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

neclimdul’s picture

Status: Reviewed & tested by the community » Needs work

need re-roll

hussainweb’s picture

Status: Needs work » Needs review
FileSize
54.53 KB

Here is a reroll attempt.

Status: Needs review » Needs work

The last submitted patch, uuid-service-1969572-131.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
56.19 KB

Found a couple of more places, which does not need the Uuid use statement anymore and rerolled the patch.

Status: Needs review » Needs work

The last submitted patch, uid-1969572-131.patch, failed testing.

xjm’s picture

We don't add the "Needs change notification" tag until the patch is committed. :)

neclimdul’s picture

Status: Needs work » Needs review
FileSize
56.37 KB

reroll 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.

Status: Needs review » Needs work

The last submitted patch, uuid-service-1969572-136.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
614 bytes
56.74 KB

Let's fix this tiny kind of unimportant line.

neclimdul’s picture

yeah... saw that and wrote the same patch and was about to upload it :)

Status: Needs review » Needs work

The last submitted patch, uuid-1969572-138.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
6.17 KB
4.21 KB
61.41 KB

Fix 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.

tim.plunkett’s picture

@@ -2,7 +2,7 @@
- * Definition of Drupal\Component\Uuid\Php.
+ * Contains \Drupal\Component\Uuid\Php.

Out of scope, but whatever

@@ -50,13 +50,23 @@ class ConfigContext implements ContextInterface {
+   * The Uuid service.
...
+   *   The Uuid service.

@@ -28,13 +29,23 @@ class ConfigContextFactory {
+   * The Uuid service.

@@ -42,6 +42,13 @@ class ConfigStorageController extends EntityStorageControllerBase {
+   * The Uuid service.

When its in a comment, please "UUID" (I didn't highlight every one, but there are a bunch)

@@ -376,8 +375,9 @@ public function createDuplicate() {
+      // @todo Inject the uuid service into the storage controller once
+      //    possible.

@@ -789,7 +790,9 @@ public function createDuplicate() {
+      // @todo Inject the uuid service into the storage controller once
+      //    possible.

This is not a storage controller, its the Entity class.

@@ -95,14 +102,15 @@ class ConfigSync implements ControllerInterface, FormInterface {
+   * @param \Drupal\Core\Entity\EntityManager $entity_manger

$entity_manger? Might as well leave it alone honestly...

@@ -129,7 +128,7 @@ function _image_update_get_style_with_effects(array $style) {
diff --git a/core/modules/image/image.install.rej b/core/modules/image/image.install.rej

diff --git a/core/modules/image/image.install.rej b/core/modules/image/image.install.rej
new file mode 100644

new file mode 100644
index 0000000..d531acb

index 0000000..d531acb
--- /dev/null

--- /dev/null
+++ b/core/modules/image/image.install.rej

+++ b/core/modules/image/image.install.rej
@@ -0,0 +1,10 @@

@@ -0,0 +1,10 @@
+diff a/core/modules/image/image.install b/core/modules/image/image.install	(rejected hunks)
+@@ -130,7 +129,7 @@ function _image_update_get_style_with_effects(array $style) {
+     $effect['data'] = unserialize($effect['data']);
+ ¶
+     // Generate a unique image effect ID for the effect.
+-    $uuid = new Uuid();
++    $uuid = Drupal::service('uuid');
+     $effect['ieid'] = $uuid->generate();
+ ¶
+     $effects[$effect['ieid']] = $effect;
diff --git a/core/modules/image/image.module.rej b/core/modules/image/image.module.rej

diff --git a/core/modules/image/image.module.rej b/core/modules/image/image.module.rej
new file mode 100644

new file mode 100644
index 0000000..86815b7

index 0000000..86815b7
--- /dev/null

--- /dev/null
+++ b/core/modules/image/image.module.rej

+++ b/core/modules/image/image.module.rej
@@ -0,0 +1,18 @@

@@ -0,0 +1,18 @@
+diff a/core/modules/image/image.module b/core/modules/image/image.module	(rejected hunks)
+@@ -9,7 +9,6 @@
+ use Drupal\Core\Language\Language;
+ use Drupal\field\Plugin\Core\Entity\Field;
+ use Drupal\field\Plugin\Core\Entity\FieldInstance;
+-use Drupal\Component\Uuid\Uuid;
+ use Drupal\file\Plugin\Core\Entity\File;
+ use Drupal\image\ImageStyleInterface;
+ use Drupal\image\Plugin\Core\Entity\ImageStyle;
+@@ -542,7 +541,7 @@ function image_effect_save($style, &$effect) {
+ ¶
+   // Generate a unique image effect ID for a new effect.
+   if (empty($effect['ieid'])) {
+-    $uuid = new Uuid();
++    $uuid = Drupal::service('uuid');
+     $effect['ieid'] = $uuid->generate();
+   }
+   $style->effects[$effect['ieid']] = $effect;
diff --git a/core/modules/image/lib/Drupal/image/ImageEffectBag.php b/core/modules/image/lib/Drupal/image/ImageEffectBag.php

Whoops!

@@ -7,7 +7,6 @@
-use Drupal\Component\Uuid\Uuid;

@@ -55,7 +54,7 @@ public function testDefaultValues() {
+    $this->assertTrue(\Drupal\Component\Uuid\Uuid::isValid($entity->uuid->value), format_string('%entity_type: Default UUID', array('%entity_type' => $entity_type)));

Should leave this use statement in to simplify this line

@@ -40,19 +41,21 @@ class UserStorageController extends DatabaseStorageControllerNG implements UserS
-   * @param string $entityType
+   * @param string $entity_type

Not doing yourself any favors here on rerolls...

neclimdul’s picture

The 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.

Status: Needs review » Needs work
Issue tags: -Entity system, -PHPUnit, -API change, -WSSCI Conversion, -RTBC July 1

The last submitted patch, uuid-service-1969572-143.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#143: uuid-service-1969572-143.patch queued for re-testing.

Random missing table fail.

Status: Needs review » Needs work
Issue tags: +Entity system, +PHPUnit, +API change, +WSSCI Conversion, +RTBC July 1

The last submitted patch, uuid-service-1969572-143.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
60.27 KB

Because i'm lazy no interdiff but there where just 2 small config() -> Drupal::config() changes rebase handled.

dawehner’s picture

  1. +++ b/core/modules/image/image.install
    index 7d0c8eb..e7d9899 100644
    --- a/core/modules/image/lib/Drupal/image/ImageEffectBag.php
    
    --- a/core/modules/image/lib/Drupal/image/ImageEffectBag.php
    +++ b/core/modules/image/lib/Drupal/image/ImageEffectBag.php
    
    +++ b/core/modules/image/lib/Drupal/image/ImageEffectBag.php
    @@ -51,7 +50,7 @@ public function removeInstanceID($instance_id) {
    +      $uuid_generator = \Drupal::service('uuid');
    

    It 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.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldDefaultValueTest.php
    @@ -55,7 +55,7 @@ public function testDefaultValues() {
    +    $this->assertTrue(Uuid::isValid($entity->uuid->value), format_string('%entity_type: Default UUID', array('%entity_type' => $entity_type)));
    

    We could also switch to String::format

  3. +++ b/core/tests/Drupal/Tests/Component/Uuid/UuidTest.php
    @@ -0,0 +1,103 @@
    +    foreach ($this->uuidInstances as $instance) {
    ...
    +    foreach ($this->uuidInstances as $instance) {
    ...
    +    foreach ($this->uuidInstances as $instance) {
    ...
    +    foreach ($this->uuidInstances as $instance) {
    

    Theoretically all this foreach could be solved with dataProviders but I am not sure whether this concept if valid in this case.

  4. +++ b/core/tests/Drupal/Tests/Component/Uuid/UuidTest.php
    @@ -0,0 +1,103 @@
    +      $this->assertTrue((bool) Uuid::isValid($uuid_fqdn));
    +      $this->assertTrue((bool) Uuid::isValid($uuid_min));
    +      $this->assertTrue((bool) Uuid::isValid($uuid_max));
    ...
    +      $this->assertFalse((bool) Uuid::isValid($invalid_format));
    +      $this->assertFalse((bool) Uuid::isValid($invalid_length));
    

    isValid should already return a boolean value

tim.plunkett’s picture

I'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.

ParisLiakos’s picture

Status: Needs review » Needs work
star-szr’s picture

Issue tags: -WSSCI Conversion

Tag fix, WSSCI -> WSCCI.

star-szr’s picture

Issue tags: +WSCCI-conversion

Sorry for the noise, didn't check autocomplete.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
6.1 KB
59.12 KB

Finally another patch. Rerolled and then interdiff from there with the changes from #148 minus #149.

neclimdul’s picture

Re-roll with "Drupal" to "\Drupal" changes.

Status: Needs review » Needs work
Issue tags: -Entity system, -PHPUnit, -API change, -WSCCI-conversion, -RTBC July 1

The last submitted patch, drupal8.base-system.1969572-154.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Entity system, +PHPUnit, +API change, +WSCCI-conversion, +RTBC July 1

The last submitted patch, drupal8.base-system.1969572-154.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
59.16 KB

I can't recreate this... recreated the patch just to make sure.

dawehner’s picture

  1. +++ b/core/tests/Drupal/Tests/Component/Uuid/UuidTest.php
    @@ -0,0 +1,92 @@
    + *
    + * @group UUID
    

    It would be cool to have also one @group Drupal in order to be able to just run all of these tests.

  2. +++ b/core/tests/Drupal/Tests/Component/Uuid/UuidTest.php
    @@ -0,0 +1,92 @@
    +   * Tests generating a UUID.
    +   *
    +   * @dataProvider uuidInstances
    +   */
    +  public function testGenerateUuid(UuidInterface $instance) {
    ...
    +  /**
    +   * Tests that generated UUIDs are unique.
    +   *
    +   * @dataProvider uuidInstances
    +   */
    +  public function testUuidIsUnique(UuidInterface $instance) {
    +    $this->assertNotEquals($instance->generate(), $instance->generate());
    +  }
    

    Some short documentation lines could be added here.

jibran’s picture

+++ b/core/tests/Drupal/Tests/Component/Uuid/UuidTest.php
@@ -0,0 +1,92 @@
+    $instances[][] = new \Drupal\Component\Uuid\Php();

I think we should declare $instances array() first.

socketwench’s picture

Rerolled, incorporated changes from #160.

neclimdul’s picture

reroll 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.

ParisLiakos’s picture

Assigned: hussainweb » Unassigned
FileSize
4.04 KB
59.82 KB

so lets fix the phpunit test a bit. otherwise its ready to go

jibran’s picture

#160 is still not fixed in last patch.

ParisLiakos’s picture

yes, 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

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @ParisLiakos for the changes. As #159 is addressed and as per #163 this is good to go so RTBC for me.

neclimdul’s picture

Assigned: Unassigned » neclimdul
Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/tests/Drupal/Tests/Component/Uuid/UuidTest.php
    @@ -0,0 +1,116 @@
    +  public function testValidation($uuid, $is_valid) {
    +    $this->assertEquals($is_valid, Uuid::isValid($uuid));
    +  }
    +
    

    These changes removed the messages added to address #159.

  2. I'll reroll.

  3. +++ b/core/tests/Drupal/Tests/Component/Uuid/UuidTest.php
    @@ -0,0 +1,116 @@
    +
    +    $instances = array(
    +      array(new Php()),
    +    );
    +
    +    // If valid PECL extensions exists add to list.
    +    if (function_exists('uuid_create') && !function_exists('uuid_make')) {
    +      $instances[][] = new Pecl();
    +    }
    +
    +    // If we are on Windows add the com implementation as well.
    +    if (function_exists('com_create_guid')) {
    +      $instances[][] = new Com();
    +    }
    +
    +    return $instances;
    

    this seems less clear then the FQ class name.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
59.65 KB

Actually 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.

Status: Needs review » Needs work

The last submitted patch, drupal8.base-system.1969572-168.patch, failed testing.

neclimdul’s picture

hunk 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.

neclimdul’s picture

Status: Needs work » Needs review

lets run testbot like a pro.

ParisLiakos’s picture

Actually 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.

You 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

These changes removed the messages added to address #159.

PHPunit gives those info by default. Here how it looks when one data provider set fails:

There was 1 failure:

1) Drupal\Tests\Component\Uuid\UuidTest::testValidation with data set #0 ('6ba7b810-9dad-11d1-80b4-00c04fd430c8', false)
Failed asserting that true matches expected false.

Which is a lot better than concatenating strings and variables ;)

ParisLiakos’s picture

sigh

neclimdul’s picture

I 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.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

agreed

webchick’s picture

Title: Make Uuid a service » Needs change notice: Make Uuid a service
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed and pushed to 8.x. Thanks!

This'll need a change notice.

neclimdul’s picture

Assigned: neclimdul » Unassigned

Did 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

Gábor Hojtsy’s picture

Title: Needs change notice: Make Uuid a service » Make Uuid a service
Priority: Major » Normal
Issue summary: View changes
Status: Active » Fixed
Issue tags: -Needs change record

Added a change notice at https://drupal.org/node/2184281 which covers this one and the original issue to add UUIDs.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.