Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Issue tags: +node ng
Berdir’s picture

Status: Active » Needs review
FileSize
7.65 KB

Here's a first patch for this.

Seems to work fine, let's see if there are any test fails.

Not happy about the test files that I used, I need real images and drupalGetTestFiles() is not available in DUBT, which is something that we might want to change?

das-peter’s picture

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageItemTest.php
@@ -0,0 +1,113 @@
+  public function testImageItem() {
+    // Create a test entity with the
+    $entity = entity_create('entity_test', array());
+    $entity->image_test->fid = $this->image->id();

with what? :)
Would be "// Create a test entity with the image field set." okay?
Attached patch only adds image field set.

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageItemTest.php
@@ -0,0 +1,113 @@
+    // The width and height is only updated when width is not set.
+    $entity->image_test->width = NULL;

Is this behaviour intended? Shouldn't it also be triggered by changing the fid?
Could we unset the width automatically if a different fid is set in ImageItem::setValue()?

Berdir’s picture

Opened #1919088: width/height of image field not updated when fid changes for the width problem as it wasn't introduced here, it simply has never been tested like this before. Not 100% sure what's the best way to detect and re-act to that, so I think doing it in a follow-up makes sense.

Your comment change looks good to me, I think it's fine if you RTBC despite making such a small change.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Let's do this.

fago’s picture

Code looks good and I agree that it makes sense to work on the height issue in a follow-up

I found the following minor doc issues, again that can be fixed as follow-up also.

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageItemTest.php
@@ -0,0 +1,113 @@
+ * @image
+ * Contains \Drupal\image\Tests\ImageItemTest.

This should be @file ;-)

+++ b/core/modules/image/lib/Drupal/image/Type/ImageItem.php
@@ -0,0 +1,101 @@
+ * @image
+ * Contains \Drupal\image\Type\ImageItem.

again @file

Could we unset the width automatically if a different fid is set in ImageItem::setValue()?

Yeah, the references are being reworked at #1868004: Improve the TypedData API usage of EntityNG. We need to figure out on how to deal with updates best, e.g. maybe we should make it possible for the parent to get notified when the child changes... I guess this would be useful in many cases, e.g. also if we want to track which entity fields change.

Anonymous’s picture

Couldn't ImageItem extend FileItem? There seems to be heavy overlap between them.

Berdir’s picture

Hm. All *Item class look similar.

I'd prefer to get it in first and then look if we can unify them afterwards I don't think we can actually save much code at the moment here. They have the same two methods, yes but those are different as they are defining and checking different properties.

Anonymous’s picture

For me, it's not so much about code reduction as about the Serializer's Normalizer having a way to know that two field item types have some kind of relationship.

amateescu’s picture

Actually, there is a lot of code that can be saved if we extend FileItem and I think it makes a lot of sense to do so in this patch.

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

The last submitted patch, image-field-type-1839066-10.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#10: image-field-type-1839066-10.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +node ng

The last submitted patch, image-field-type-1839066-10.patch, failed testing.

amateescu’s picture

Assigned: Unassigned » plach

I wonder if @plach can shed some light on that failing test. I've tested #3 locally and it has the same fail, so the problem is not with the new patch :/

amateescu’s picture

Status: Needs work » Needs review

#3: image-field-type-1839066-3.patch queued for re-testing.

Asked for a re-test on #3, just to be sure I'm not crazy.

fago’s picture

I'd be happy to see this extending the FileItem class, but #10 demonstrates that the sub-type relationship is not given. We do not have all properties of FileItem in the ImageItem, so it's not a candidate for the sub-type relationship. We can make it so though, but I agree that we should move this to its own issue.

I take a look on whether I can find the source of the test fails.

fago’s picture

FileSize
634 bytes
8.29 KB

ok, looks like we have multiple problems here.

1) We create a new translation without providing a value for a translatable property, i.e. "name" of the test entity. This would have to save as NULL, but our schema says that's not valid. Thus, we need to fix our schema here. I've attached a short-gap fix for that in the updated patch.
I think we should generally take care of our schema definitions and make sure they match the API. I've commented on that here: http://drupal.org/node/1777956#comment-7156374

2) It looks like the BCEntity decorator results in a new "und" translation being created, this is why tests still fail. The storage controller thinks there should be a "und" translation, while there isn't. Thus, we need to make sure the BCEntity takes care of removing un-used und values afterwards or take this into account when calculating availableTranslationLanguages().

Status: Needs review » Needs work

The last submitted patch, d8_image.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.63 KB

Re-roll of #17 without the entity_test.install change. I confirmed that this passed with the EntityBCDecorator improvements from #1818556: Convert nodes to the new Entity Field API

Status: Needs review » Needs work

The last submitted patch, image-field-type-1839066-19.patch, failed testing.

fago’s picture

Looks like the hunk from #17 is still required? (what makes sense)

Berdir’s picture

No, this is supposed to fail. It should pass once the Node NG patch is in.

fago’s picture

I see. If it passes with a non-NULL name, that's a bug though. But it's not related to this, so if not required here it should become its own issue.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -node ng

#19: image-field-type-1839066-19.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, image-field-type-1839066-19.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +node ng

#17: d8_image.patch queued for re-testing.

Berdir’s picture

Re-roll of #17, das-peter said that it is working for him but it fails for me. Let's try again.

I got confused and tested the wrong test before, the EntityBCDecorator changes don't fix it for me, at least.

Status: Needs review » Needs work

The last submitted patch, image-field-type-1839066-27.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
760 bytes
10.98 KB

Weird. I don't understand why those values are only set in one single case but excluding them from the list makes it work.

@amateesu also said that he can't reproduce the test failures that I have on php 5.4, so it's not that.

jibran’s picture

Status: Needs review » Needs work
+++ b/sites/default/default.settings.phpundefined
@@ -587,22 +587,19 @@
  *
  * The options below return a simple, fast 404 page for URLs matching a
  * specific pattern:
- * - $conf['system.performance]['fast_404']['exclude_paths']: A regular
- *   expression to match paths to exclude, such as images generated by image
- *   styles, or dynamically-resized images. If you need to add more paths, you
- *   can add '|path' to the expression.
- * - $conf['system.performance]['fast_404']['paths']: A regular expression to
- *   match paths that should return a simple 404 page, rather than the fully
- *   themed 404 page. If you don't have any aliases ending in htm or html you
- *   can add '|s?html?' to the expression.
- * - $conf['system.performance]['fast_404']['html']: The html to return for
- *   simple 404 pages.
+ * - $conf['system.fast_404']['exclude_paths']: A regular expression to match paths to exclude,
+ *   such as images generated by image styles, or dynamically-resized images.
+ *   If you need to add more paths, you can add '|path' to the expression.
+ * - $conf['system.fast_404']['paths']: A regular expression to match paths that should return a
+ *   simple 404 page, rather than the fully themed 404 page. If you don't have
+ *   any aliases ending in htm or html you can add '|s?html?' to the expression.
+ * - $conf['system.fast_404']['html']: The html to return for simple 404 pages.
  *
  * Remove the leading hash signs if you would like to alter this functionality.
  */
-#$conf['system.performance]['fast_404']['exclude_paths'] = '/\/(?:styles)\//';
-#$conf['system.performance]['fast_404']['paths'] = '/\.(?:txt|png|gif|jpe?g|css|js|ico|swf|flv|cgi|bat|pl|dll|exe|asp)$/i';
-#$conf['system.performance]['fast_404']['html'] = '<!DOCTYPE html><html><head><title>404 Not Found</title></head><body><h1>Not Found</h1><p>The requested URL "@path" was not found on this server.</p></body></html>';
+#$conf['system.fast_404']['exclude_paths'] = '/\/(?:styles)\//';
+#$conf['system.fast_404']['paths'] = '/\.(?:txt|png|gif|jpe?g|css|js|ico|swf|flv|cgi|bat|pl|dll|exe|asp)$/i';
+#$conf['system.fast_404']['html'] = '<!DOCTYPE html><html><head><title>404 Not Found</title></head><body><h1>Not Found</h1><p>The requested URL "@path" was not found on this server.</p></body></html>';
 
 /**

I think this got mix up in merge. #1934716: [Followup] Merge system.fast_404 config object into system.performance

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.51 KB

Yep, re-rolled.

fago’s picture

Looks good.

I think this is ready and the last issue needed for #1732730: [meta] Implement the new entity field API for all field types. Reviews/RTBC anyone?

plach’s picture

Overall looks good to me, I have two minor complaints and a question:

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageItemTest.php
@@ -0,0 +1,112 @@
+ * @image

Search/replace leftover? ;)

+++ b/core/modules/image/lib/Drupal/image/Type/ImageItem.php
@@ -0,0 +1,105 @@
+        'type' => 'integer',

Why does this not need to be declared as an entity reference?

+++ b/core/modules/image/lib/Drupal/image/Type/ImageItem.php
@@ -0,0 +1,105 @@
+    if (!is_array($values)) {
+      $values = array('entity' => $values);

If I'm not mistaken this is not covered by the test.

Dave Reid’s picture

Is this where we should be enforcing that height and width properties should not actually be editable, and they're in-fact derivative from changing the fid value?

Berdir’s picture

Basically yes, also automatically updating it when the file changes. See the @todo in the test. However, I'd like to do that in a follow-up issue, depends on some Entity Field API issues that allow the parent (e.g. the file item) to properly re-act to changes in it's properties.

Berdir’s picture

Re-roll, fixed the @image and added tests for that part.

fid is not an entity reference because it's on the same level, it has fid and entity properties like an entity reference has target_id and entity. It's a different kind of entity reference, one that is specific to files. It probably should become an entity reference at some point or extend it but that would also imply to change all access to it (target_id, not fid) and the storage.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Cool

Edit: Actually I missed the following:

+++ b/core/modules/image/lib/Drupal/image/Type/ImageItem.php
@@ -0,0 +1,105 @@
+    // Treat the values as property value of the entity field, if no array
+    // is given.

Comment not wrapping correctly.

I guess we can fix this on commit or if this needs a reroll.

Berdir’s picture

#36: image-field-type-1839066-36.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

plach’s picture

plach’s picture

Assigned: plach » Unassigned

wtf

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