See ConfigContext::setUuid()

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

We should inject dependencies, not hardcode em
Files: 
CommentFileSizeAuthor
#174 drupal8.base-system.1969572-174.patch58.39 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 58,525 pass(es).
[ View ]
#174 interdiff.txt2.67 KBneclimdul
#170 drupal8.base-system.1969572-170.patch58.04 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 58,958 pass(es).
[ View ]
#170 interdiff.txt1.19 KBneclimdul
#168 drupal8.base-system.1969572-168.patch59.65 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.base-system.1969572-168.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#168 interdiff.txt2.62 KBneclimdul
#165 drupal-uuid_as_service-165.patch59.85 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,633 pass(es).
[ View ]
#165 interdiff.txt602 bytesParisLiakos
#163 drupal-uuid_as_service-163.patch59.82 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 59,029 pass(es).
[ View ]
#163 interdiff.txt4.04 KBParisLiakos
#162 drupal8.base-system.1969572-162.patch59.61 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 58,704 pass(es).
[ View ]
#162 interdiff.txt2.31 KBneclimdul
#161 drupal8.base-system.1969572-161.patch58.98 KBsocketwench
FAILED: [[SimpleTest]]: [MySQL] 58,486 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#158 drupal8.base-system.1969572-158.patch59.16 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 58,681 pass(es).
[ View ]
#154 drupal8.base-system.1969572-154.patch59.63 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] 58,221 pass(es), 702 fail(s), and 61 exception(s).
[ View ]
#153 drupal8.base-system.1969572-153.patch59.12 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 58,815 pass(es).
[ View ]
#153 interdiff.txt6.1 KBneclimdul
#147 uuid-service-1969572-147.patch60.27 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 58,185 pass(es).
[ View ]
#143 interdiff.txt14.73 KBneclimdul
#143 uuid-service-1969572-143.patch60.26 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch uuid-service-1969572-143_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#141 uuid-service-1969572-141.patch61.41 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 57,927 pass(es).
[ View ]
#141 interdiff.txt4.21 KBneclimdul
#141 interdiff2.txt6.17 KBneclimdul
#138 uuid-1969572-138.patch56.74 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,814 pass(es), 31 fail(s), and 26 exception(s).
[ View ]
#138 interdiff.txt614 bytesdawehner
#136 uuid-service-1969572-136.patch56.37 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#133 uid-1969572-131.patch56.19 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,696 pass(es), 32 fail(s), and 32 exception(s).
[ View ]
#131 uuid-service-1969572-131.patch54.53 KBhussainweb
FAILED: [[SimpleTest]]: [MySQL] 55,974 pass(es), 32 fail(s), and 32 exception(s).
[ View ]
#125 uuid-service-1969572-125.patch55.89 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 56,297 pass(es).
[ View ]
#123 uuid-uuid-service-1969572-123.patch55.88 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,704 pass(es).
[ View ]
#123 interdiff.txt3.1 KBParisLiakos
#121 uuid-uuid-service-1969572-121.patch55.99 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 56,664 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#121 uuid-uuid-service-1969572-121-interdiff.txt3.02 KBBerdir
#118 interdiff.txt1.5 KBneclimdul
#118 uuid-service-1969572-118.patch53.48 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] 55,089 pass(es), 37 fail(s), and 32 exception(s).
[ View ]
#115 uuid-service-1969572-115.patch53.44 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#113 uuid-service-1969572-113.patch53.35 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch uuid-service-1969572-113.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#111 uuid-service-1969572-111.patch52.9 KBhussainweb
FAILED: [[SimpleTest]]: [MySQL] 56,142 pass(es), 27 fail(s), and 25 exception(s).
[ View ]
#108 uuid-service-1969572-109.patch52.34 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 58,733 pass(es).
[ View ]
#98 uuid-service-1969572-98.patch51.56 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] 57,947 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#98 interdiff.txt4.89 KBneclimdul
#94 uuid-service-1969572-94.patch52.73 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#93 uuid-service-1969572-93.patch52.76 KBhussainweb
PASSED: [[SimpleTest]]: [MySQL] 58,054 pass(es).
[ View ]
#91 uuid-service-1969572-91.patch52.76 KBhussainweb
FAILED: [[SimpleTest]]: [MySQL] 58,150 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#89 uuid-service-1969572-89.patch53.21 KBhussainweb
FAILED: [[SimpleTest]]: [MySQL] 57,583 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#79 uuid-service-1969572-79.patch51.79 KBhussainweb
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#73 1969572-73.patch51.67 KBpwieck
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php.
[ View ]
#68 1969572-68.patch51.85 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,105 pass(es).
[ View ]
#68 interdiff.txt1.83 KBParisLiakos
#56 1969572-56.patch51.76 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 57,678 pass(es).
[ View ]
#52 1969572-52.patch51.92 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,551 pass(es).
[ View ]
#52 interdiff-1969572-52.txt4 KBdamiankloip
#50 uuid-1969572-50.patch49.24 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#50 interdiff.txt1.61 KBdawehner
#46 uuid-1969572-46.patch47.36 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#46 interdiff.txt1.45 KBdawehner
#45 uuid-1969572.45.interdiff.txt1.19 KBlarowlan
#45 uuid-1969572.45.patch47.14 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,325 pass(es).
[ View ]
#42 1969572-42.patch45.93 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 56,936 pass(es).
[ View ]
#42 interdiff-1969572-42.txt742 bytesdamiankloip
#40 1969572-40.patch45.39 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 54,558 pass(es), 30 fail(s), and 30 exception(s).
[ View ]
#34 1969572-34.patch45.1 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,404 pass(es).
[ View ]
#34 interdiff-1969572-34.txt6.8 KBdamiankloip
#30 1969572-31.patch42.28 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,109 pass(es).
[ View ]
#30 interdiff-1969572-31.patch9.88 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1969572-31.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#25 drupal-1969572-25.patch34.21 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#24 drupal-1969572-24.patch31.17 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#24 interdiff.txt5.7 KBdawehner
#21 uuid-1969572.21.interdiff.txt691 byteslarowlan
#21 uuid-1969572.21.patch34.18 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch uuid-1969572.21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 uuid-1969572.17.interdiff.txt0 byteslarowlan
#17 uuid-1969572.17.patch34.19 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,661 pass(es).
[ View ]
#14 uuid-1969572.14.interdiff.txt4.91 KBlarowlan
#14 uuid-1969572.14.patch29.57 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#12 uuid-1969572.12.interdiff.txt1.18 KBlarowlan
#12 uuid-1969572.12.patch24.66 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#9 uuid-1969572.8.interdiff.txt1.15 KBlarowlan
#9 uuid-1969572.8.patch24.1 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#5 uuid-1969572.5.interdiff.txt9.17 KBlarowlan
#5 uuid-1969572.5.patch23.61 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#2 uuid-1969572.2.patch16.2 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Tagging

Status:Active» Needs review
StatusFileSize
new16.2 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

First cut

Status:Needs review» Needs work

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

/**
* 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.

Status:Needs work» Needs review
StatusFileSize
new23.61 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new9.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.

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

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

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

StatusFileSize
new24.1 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new1.15 KB

Registers in the installer

Status:Needs review» Needs work

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

+++ 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"

Status:Needs work» Needs review
StatusFileSize
new24.66 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new1.18 KB

Should install now

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new29.57 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new4.91 KB

This time?
Installs locally

Status:Needs review» Needs work

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

Going to move the unit test to phpunit instead of UnitTestBase

Status:Needs work» Needs review
StatusFileSize
new34.19 KB
PASSED: [[SimpleTest]]: [MySQL] 55,661 pass(es).
[ View ]
new0 bytes

Moves to php unit test

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

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

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.

StatusFileSize
new34.18 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch uuid-1969572.21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new691 bytes

Removes Unit from test class as per #18

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.

StatusFileSize
new5.7 KB
new31.17 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Let's play around with the idea of #22

StatusFileSize
new34.21 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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.

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

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

StatusFileSize
new9.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1969572-31.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new42.28 KB
PASSED: [[SimpleTest]]: [MySQL] 57,109 pass(es).
[ View ]

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.

Status:Needs work» Needs review

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

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

StatusFileSize
new6.8 KB
new45.1 KB
PASSED: [[SimpleTest]]: [MySQL] 57,404 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

thank you!

I'm sorry for this mistake!

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

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

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Needs review
StatusFileSize
new45.39 KB
FAILED: [[SimpleTest]]: [MySQL] 54,558 pass(es), 30 fail(s), and 30 exception(s).
[ View ]

Rerolled

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new742 bytes
new45.93 KB
PASSED: [[SimpleTest]]: [MySQL] 56,936 pass(es).
[ View ]

Sorry field module.

Status:Needs review» Reviewed & tested by the community

back to rtbc then:)

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

Issue tags:-Needs reroll
StatusFileSize
new47.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,325 pass(es).
[ View ]
new1.19 KB

Lets see how this goes.

StatusFileSize
new1.45 KB
new47.36 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.61 KB
new49.24 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new4 KB
new51.92 KB
PASSED: [[SimpleTest]]: [MySQL] 57,551 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Good catch!

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

Working on re-roll

Status:Needs work» Needs review
StatusFileSize
new51.76 KB
PASSED: [[SimpleTest]]: [MySQL] 57,678 pass(es).
[ View ]

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.

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.

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.

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.

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.

Ready for review!

Status:Needs review» Reviewed & tested by the community

back to rtbc

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

Status:Needs work» Needs review
StatusFileSize
new1.83 KB
new51.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,105 pass(es).
[ View ]

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

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.

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

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

Assigned:Unassigned» pwieck

Working on re-roll

Assigned:pwieck» Unassigned
StatusFileSize
new51.67 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php.
[ View ]

Re-rolled

Status:Needs work» Needs review

changing status

Status:Needs review» Needs work

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

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);

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.

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

Status:Needs work» Needs review
StatusFileSize
new51.79 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

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.

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.

Doing a manual test now just to make sure.

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

@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?

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new53.21 KB
FAILED: [[SimpleTest]]: [MySQL] 57,583 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new52.76 KB
FAILED: [[SimpleTest]]: [MySQL] 58,150 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Attaching changes as per #90. Thanks!

Status:Needs review» Needs work

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

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new52.76 KB
PASSED: [[SimpleTest]]: [MySQL] 58,054 pass(es).
[ View ]

Fixing a small typo...

StatusFileSize
new52.73 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Status:Needs review» Needs work

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

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

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.

StatusFileSize
new4.89 KB
new51.56 KB
FAILED: [[SimpleTest]]: [MySQL] 57,947 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

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.

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.

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.

#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

StatusFileSize
new52.34 KB
PASSED: [[SimpleTest]]: [MySQL] 58,733 pass(es).
[ View ]

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

Status:Needs work» Needs review

woops

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

This certainly need a rerole.

Status:Needs work» Needs review
StatusFileSize
new52.9 KB
FAILED: [[SimpleTest]]: [MySQL] 56,142 pass(es), 27 fail(s), and 25 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new53.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch uuid-service-1969572-113.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

lets try this. needed another re-roll anyways.

Status:Needs review» Needs work

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

StatusFileSize
new53.44 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new53.48 KB
FAILED: [[SimpleTest]]: [MySQL] 55,089 pass(es), 37 fail(s), and 32 exception(s).
[ View ]
new1.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.

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.

Status:Needs work» Needs review
StatusFileSize
new3.02 KB
new55.99 KB
FAILED: [[SimpleTest]]: [MySQL] 56,664 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

This should be all of them.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.1 KB
new55.88 KB
PASSED: [[SimpleTest]]: [MySQL] 56,704 pass(es).
[ View ]

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.

StatusFileSize
new55.89 KB
PASSED: [[SimpleTest]]: [MySQL] 56,297 pass(es).
[ View ]

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.

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.

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work

need re-roll

Status:Needs work» Needs review
StatusFileSize
new54.53 KB
FAILED: [[SimpleTest]]: [MySQL] 55,974 pass(es), 32 fail(s), and 32 exception(s).
[ View ]

Here is a reroll attempt.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new56.19 KB
FAILED: [[SimpleTest]]: [MySQL] 55,696 pass(es), 32 fail(s), and 32 exception(s).
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new56.37 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new614 bytes
new56.74 KB
FAILED: [[SimpleTest]]: [MySQL] 57,814 pass(es), 31 fail(s), and 26 exception(s).
[ View ]

Let's fix this tiny kind of unimportant line.

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.

Status:Needs work» Needs review
StatusFileSize
new6.17 KB
new4.21 KB
new61.41 KB
PASSED: [[SimpleTest]]: [MySQL] 57,927 pass(es).
[ View ]

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.

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

StatusFileSize
new60.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch uuid-service-1969572-143_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new14.73 KB

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.

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.

Status:Needs work» Needs review
StatusFileSize
new60.27 KB
PASSED: [[SimpleTest]]: [MySQL] 58,185 pass(es).
[ View ]

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

  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

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.

Status:Needs review» Needs work

Issue tags:-WSSCI Conversion

Tag fix, WSSCI -> WSCCI.

Issue tags:+WSCCI-conversion

Sorry for the noise, didn't check autocomplete.

Status:Needs work» Needs review
StatusFileSize
new6.1 KB
new59.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,815 pass(es).
[ View ]

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

StatusFileSize
new59.63 KB
FAILED: [[SimpleTest]]: [MySQL] 58,221 pass(es), 702 fail(s), and 61 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new59.16 KB
PASSED: [[SimpleTest]]: [MySQL] 58,681 pass(es).
[ View ]

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

  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.

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

StatusFileSize
new58.98 KB
FAILED: [[SimpleTest]]: [MySQL] 58,486 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Rerolled, incorporated changes from #160.

StatusFileSize
new2.31 KB
new59.61 KB
PASSED: [[SimpleTest]]: [MySQL] 58,704 pass(es).
[ View ]

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.

Assigned:hussainweb» Unassigned
StatusFileSize
new4.04 KB
new59.82 KB
PASSED: [[SimpleTest]]: [MySQL] 59,029 pass(es).
[ View ]

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

#160 is still not fixed in last patch.

StatusFileSize
new602 bytes
new59.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,633 pass(es).
[ View ]

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new2.62 KB
new59.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.base-system.1969572-168.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

StatusFileSize
new1.19 KB
new58.04 KB
PASSED: [[SimpleTest]]: [MySQL] 58,958 pass(es).
[ View ]

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.

Status:Needs work» Needs review

lets run testbot like a pro.

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 ;)

sigh

StatusFileSize
new2.67 KB
new58.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,525 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

agreed

Title:Make Uuid a serviceNeeds 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.

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

Title:Needs change notice: Make Uuid a serviceMake 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.