Currently there are 7 test entity types all using the same class.
This was hacky and they should have been subclasses to begin with.

Because of #1763974: Convert entity type info into plugins, it's now required, and that issue makes the necessary changes to their structure, but exposes and adds some documentation inconsistencies.

CommentFileSizeAuthor
#123 convert-test-entity-to-entity-test-1822000-121.patch175.38 KBBerdir
#123 convert-test-entity-to-entity-test-1822000-121-interdiff.txt9.16 KBBerdir
#112 convert-test-entity-to-entity-test-1822000-112.patch170.21 KBBerdir
#112 convert-test-entity-to-entity-test-1822000-112-interdiff.txt4.18 KBBerdir
#110 convert-test-entity-to-entity-test-1822000-110.patch170.57 KBBerdir
#108 interdiff.txt819 bytesplach
#108 convert-test-entity-to-entity-test-1822000-108.patch183.08 KBplach
#105 convert-test-entity-to-entity-test-1822000-105.patch185.9 KBBerdir
#105 convert-test-entity-to-entity-test-1822000-105-interdiff.txt9.46 KBBerdir
#104 interdiff.txt2.25 KBplach
#95 convert-test-entity-to-entity-test-1822000-95.patch184.5 KBBerdir
#95 convert-test-entity-to-entity-test-1822000-95-interdiff.txt3.39 KBBerdir
#93 convert-test-entity-to-entity-test-1822000-93.patch184.28 KBBerdir
#93 convert-test-entity-to-entity-test-1822000-93-interdiff.txt5.07 KBBerdir
#91 convert-test-entity-to-entity-test-1822000-91.patch182.65 KBBerdir
#91 convert-test-entity-to-entity-test-1822000-91-interdiff.txt4.1 KBBerdir
#90 interdiff.txt3.26 KBplach
#88 convert-test-entity-to-entity-test-1822000-88.patch181.4 KBBerdir
#88 convert-test-entity-to-entity-test-1822000-88-interdiff.txt7.44 KBBerdir
#86 convert-test-entity-to-entity-test-1822000-86.patch178.98 KBBerdir
#86 convert-test-entity-to-entity-test-1822000-86-interdiff.txt12.64 KBBerdir
#85 interdiff.txt2.37 KBplach
#82 convert-test-entity-to-entity-test-1822000-82.patch174.28 KBBerdir
#78 convert-test-entity-to-entity-test-1822000-78.patch177.28 KBBerdir
#78 convert-test-entity-to-entity-test-1822000-78-interdiff.txt12.96 KBBerdir
#75 convert-test-entity-to-entity-test-1822000-75.patch167.24 KByched
#75 interdiff.txt3.4 KByched
#69 convert-test-entity-to-entity-test-1822000-69.patch167.26 KBBerdir
#69 convert-test-entity-to-entity-test-1822000-69-interdiff.txt3.2 KBBerdir
#67 convert-test-entity-to-entity-test-1822000-67.patch168.79 KBBerdir
#63 convert-test-entity-to-entity-test-1822000-63.patch267.15 KBBerdir
#63 convert-test-entity-to-entity-test-1822000-63-interdiff.txt1.73 KBBerdir
#61 convert-test-entity-to-entity-test-1822000-61.patch267.56 KBBerdir
#61 convert-test-entity-to-entity-test-1822000-61-interdiff.txt2.01 KBBerdir
#59 interdiff.txt24.66 KBamateescu
#57 convert-test-entity-to-entity-test-1822000-57.patch271.69 KBBerdir
#57 convert-test-entity-to-entity-test-1822000-57-interdiff.txt26.4 KBBerdir
#55 convert-test-entity-to-entity-test-1822000-55.patch258.27 KBBerdir
#55 convert-test-entity-to-entity-test-1822000-55-interdiff.txt29.67 KBBerdir
#51 convert-test-entity-to-entity-test-1822000-51.patch239.3 KBBerdir
#48 convert-test-entity-to-entity-test-1822000-48.patch240.34 KBBerdir
#48 convert-test-entity-to-entity-test-1822000-48-interdiff.txt4.85 KBBerdir
#46 convert-test-entity-to-entity-test-1822000-46.patch237.88 KBBerdir
#46 convert-test-entity-to-entity-test-1822000-46-interdiff.txt33.68 KBBerdir
#44 convert-test-entity-to-entity-test-1822000-44.patch233.2 KBBerdir
#44 convert-test-entity-to-entity-test-1822000-44-interdiff.txt767 bytesBerdir
#42 convert-test-entity-to-entity-test-1822000-42.patch233.95 KBBerdir
#42 convert-test-entity-to-entity-test-1822000-42-interdiff.txt48.25 KBBerdir
#36 convert-test-entity-to-entity-test-1822000-36.patch254.68 KBBerdir
#30 convert-test-entity-to-entity-test-30.patch252.79 KBBerdir
#28 convert-test-entity-to-entity-test-1822000-28.patch208.07 KBBerdir
#26 convert-test-entity-to-entity-test-1822000-26.patch240.02 KBBerdir
#24 convert-test-entity-to-entity-test-24.patch234.14 KBBerdir
#22 convert-test-entity-to-entity-test-22.patch435.17 KBBerdir
#20 convert-test-entity-to-entity-test-20.patch431.38 KBBerdir
#18 convert-test-entity-to-entity-test-18.patch416.31 KBBerdir
#16 convert-test-entity-to-entity-test-1822000-16.patch404.7 KBBerdir
#14 convert-test-entity-to-entity-test-1822000-14.patch209.57 KBBerdir
#14 convert-test-entity-to-entity-test-1822000-14-interdiff.txt38.23 KBBerdir
#12 convert-test-entity-to-entity-test-12.patch176.58 KBBerdir
#10 convert-test-entity-to-entity-test-10.patch161.78 KBBerdir
#8 convert-test-entity-to-entity-test-1822000-8.patch141.84 KBBerdir
#5 convert-test-entity-to-entity-test.patch130.44 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

I think they should actually be moved over the entity_test.module, that's what I've been trying to do for a while but there's always been stuff that's blocking it. Right now, we still have inconsistencies in regards to revision support of them.

fago’s picture

I agree that this should probably all be converted to the entity_test entity type. I don't see a need for having duplicating test entities - if we need multiple variations of test entities we can do so in entity_test module (and I think das_peter has been working on that already).

Berdir’s picture

Exactly, #1831444: EntityNG: Support for revisions for entity save and delete operations adds generic revision support and enables revision-support for entity_test and #1833334: EntityNG: integrate a dynamic property data table handling provides generic data table support and comes with a handful test entities to test every possible combination of these capabilities.

Those combined should be able to replace most of test_entity and the relevant test code that relies on it. Two exceptions that I'm aware of:

- Bundle CRUD functions and hooks. There are issues about improving this but I'm not sure what the exact state of them is.
- Support for editing multiple entities in the same form by the field system.

Berdir’s picture

Assigned: Unassigned » Berdir

I started to work on this, but there is an insane amount of tests to to through. New tests like edit.module are using test_entity instead of entity_test which means that we need to clean those up :(

And a lot of tests need various field types to be converted to NG, I started to write patches to various ones, see #1732730: [meta] Implement the new entity field API for all field types. Please review :)

Berdir’s picture

Status: Active » Needs review
FileSize
130.44 KB

Ok, here is a start. A bunch of search and replace of test_entity/test_bundle to entity_test + fixed the field types tests. Tons of duplicated tests in there for various field types which all are doing the same strange things :)

Will still have a lot of fails remaining, but I'd like to see which ones.

Notes:
- the field_test entity types do a bunch of things that we do not yet support in entity_test, like creating bundles. In fact, the bundle API (rename, delete, ..) still lives in field.module.
- It's currently a bit confusing because some field types have been converted to NG yet and others have not. I'm working on converting the field types in parallel and need reviews there, especially on the file item one :)
- Made a bunch of conceptual changes, notably the changes to option list callback allowed values. That relied on fixed id's and revision id's because there was nothing else but that required very complicated support for creating new revisions with fixed id's. I changed to use label/uuid/uri/bundle, which are fixed and guaranteed to be different. I think that is much easier to understand now.

Berdir’s picture

Also, field_test has a bunch of specific entity types like these: "BundleKeyTestEntity BundleTestEntity CacheableTestEntity LabelCallbackTestEntity LabelTestEntity NoLabelTestEntity". I'm not sure if we should keep them and just move them over to entity_test or merge their specific things into existing entity types. E.g. add a label callback to one of the existing entity_test types? Might be cleaner to just move them over.

The label ones for example are related to the EntityPropertiesTest, that I'm moving over to the other entity tests in #1893108: Convert most entity tests to DrupalUnitTestBase, I guess I can move those as part of that.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
141.84 KB

Updated patch, a number of tests fixed but still a lot to do. Will probably be easier after the field types have been converted.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-8.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
161.78 KB

Continued to work on this a bit.

This is a lot of work. Bundles, weird stuff with revisions and multiple values that I haven't yet figured out and more.

However, most of it is stuff that we need to do anyway. Changing from test_entity to entity_test is really the easiest part.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-10.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
176.58 KB

Work in progress, worked on the field form tests.a bit and re-rolled.

Seeing some issues that I hope will get resolved with the improvements to the BC decorator from the node NG issue.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-12.patch, failed testing.

Berdir’s picture

Another work in progress patch.

Worked a bit on DatetimeFieldTest and noticed that the DateTimeItem is completely broken as value is a date and is never saved.

That file is moved because it has a wrong name (Date*T*imeFieldTest.php). Will probably have to open a separate issue for that.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-14.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
404.7 KB

Combined with the NodeNG patch to see how this affects the test failures.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-16.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
416.31 KB

Updated with the latest version of the NodeNG patch. Fixed a number of bugs in the test, added view modes for entity_test and a new revision checkbox in EntityTestFormController (this might even be something that we could provide by default?)

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-18.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
431.38 KB

Started to add bundle support to entity_test, most of the remaining test failures are related to that.

Might have a few additional failures in other places due to the changes.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-20.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
435.17 KB

Interesting, looks like entity translation permission names fall apart for entity_test_mul if the bundle doesn't match the entity_type.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-22.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
234.14 KB

Re-roll after Node NG and Field API DUBT conversion went in. Might have some additional test fails.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-24.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
240.02 KB

Added bundle definition/column to the other entity_test types and updated the entity tests accordingly.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-26.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
208.07 KB

Another re-roll and a few test fixes.

Removed the rename of the datetime test filename as it makes it hard to review the changes and it only breaks the UI, not the actual tests.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-28.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
252.79 KB

And the patch keeps getting bigger :(

Let's see how many fails are left, should have fixed quite a few.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-30.patch, failed testing.

podarok’s picture

good to see here interdiffs for easy review during process

Berdir’s picture

This is just me messing around, too early to do reviews and too early to provide interdiffs :)

podarok’s picture

anyway interdiff can be a good helper for injecting new developers and discussions

Berdir’s picture

Issue tags: +Entity Field API

Tagging.

Berdir’s picture

Status: Needs work » Needs review
FileSize
254.68 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-36.patch, failed testing.

amateescu’s picture

With @Berdir's approval, I extracted the part about bundle support for EntityTest into a separate, smaller patch in #1971668: Add bundle support to EntityTest* entity types because I need it for rewriting some Entity Reference tests.

Also, the patch from #1974474: Make sure within field_attach_*() that we are working with a BC entity would make our lives a lot easier here.

andypost’s picture

Priority: Normal » Major

Is it possible to split this tremendous patch into parts? this one stops #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API and others

webchick’s picture

Priority: Major » Normal

That's just blocking a normal refactoring, so this is also normal.

Berdir’s picture

The split-up is already happening. See #38 for two issues, one of those already got in. Other split-ups are quite possibly, e.g. the field types, for which I'm thinking about a generic base class similar to e.g. the entity translation UI tests. Most of those were obviously copied and adjusted slightly, lot's of duplicated changes necessary in there.

This is more a personal playground to see how far we're off from removing test_entity.

Berdir’s picture

Re-roll and removed the now unecessary getBCEntity() calls.

As said before, will try to extract parts of this, e.g. the field type tests into smaller issues.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-42.patch, failed testing.

Berdir’s picture

Nice try git but that wasn't what I wanted to be merged there.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-44.patch, failed testing.

Berdir’s picture

That search & replace was wrong. Ended up with "$entity()". This should be better. Note that this will possibly have more fails but should have less failing classes.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-46.patch, failed testing.

Berdir’s picture

Some more test fixes.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-48.patch, failed testing.

xjm’s picture

Priority: Normal » Critical

#1953408: Remove ArrayAccess BC layer from field config entities is apparently postponed on this, and that's critical, so... so is this now! Congratulations! :D

Berdir’s picture

Status: Needs work » Needs review
FileSize
239.3 KB

Yay ;) This is already part of the critical convert entities to NG meta, so it doesn't really matter :)

Here's a re-roll. As said above, still plan to split this up into multiple issues.

Status: Needs review » Needs work
Issue tags: -Entity Field API

The last submitted patch, convert-test-entity-to-entity-test-1822000-51.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Entity Field API

The last submitted patch, convert-test-entity-to-entity-test-1822000-51.patch, failed testing.

Berdir’s picture

Renamed a bunch of new references to test_entity/test_bundle which should fix quite a few of those test failures. Also removed the entity types completely for now that we no longer want, that might introduce a few new failures.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-55.patch, failed testing.

Berdir’s picture

Fixed some more tests, merged #1957888: Exception when a field is empty on programmatic creation which also fixes various things. Let's see what's left...

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-57.patch, failed testing.

amateescu’s picture

FileSize
24.66 KB

I worked a bit on some of the remainig failures, but I'm not done yet so uploading only an interdiff for now.

Berdir’s picture

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldAttachStorageTest.phpundefined
@@ -336,40 +333,41 @@ function testFieldAttachSaveMissingData() {
-    $entity = clone($entity_init);
+    $entity = $entity_init->createDuplicate();

This is not the same necessarly.. the clone keeps the same id's, createDuplicate() removes them.

I think it does clone to be able to call field_attach_load() and similar multiple times on the same entity.

Berdir’s picture

This should pass the options widget test. Next step: Extract all non field API test group related changes into a separate issue.

Also included most of the interdiff from above except the clone/createDuplicate8) part.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-61.patch, failed testing.

Berdir’s picture

Revert the ItemList change and instead filter in the test. That should fix some of the fails in other issues.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-63.patch, failed testing.

Berdir’s picture

As promised, extracted all changes that aren't relevant to "Field API" test group into #2004224: Convert tests using TestEntity to EntityTest (except Field API tests), to make it easier to review those parts.

andypost’s picture

Issue tags: +Needs reroll
Berdir’s picture

Title: Clean up Drupal\field_test\Plugin\Entity\Type\TestEntity » Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
168.79 KB

Initial re-roll, will probably explode :)

Also updating title.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-67.patch, failed testing.

Berdir’s picture

This should get us back to where we were before.

Remaining cases:
- Multi-Entity forms. I think @yched looked into that already in the field types issue?
- Handling of NULL/empty arrays with BC decorator, that breaks both the field attach storage and the field sql storage tests.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-69.patch, failed testing.

yched’s picture

Yup, I'll merge in the fixes for the nested form from the field type patch tomorrow.

Berdir’s picture

I tried that as well (FormTest.php, NestedFormTest.php, field_test.admin.inc changes) but wasn't able to get the nested form tests to pass, got some weird ajax add more problems where the field was missing. Is there more that I need to merge?

effulgentsia’s picture

Not sure if it's related, but while working on #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets, I found that on my local MAMP (PHP 5.4) install, NestedFormTest would fail in mysterious ways due to faulty unserializing of object references within $form_state. However, bot seemed to not have that problem, so my local failures did not happen on bot.

amateescu’s picture

For the record, I have the same failures locally as #73 on XAMPP, PHP 5.4.4.

yched’s picture

Status: Needs work » Needs review
FileSize
3.4 KB
167.24 KB

Fixes the nested form test.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-75.patch, failed testing.

Berdir’s picture

Issue tags: +Field API NG blocker

Tagging.

Berdir’s picture

Fixing the remaining form tests, working on translation test but that's above my head. Also resolved merge conflicts in config import tests.

Status: Needs review » Needs work
Issue tags: -Entity Field API, -Field API NG blocker

The last submitted patch, convert-test-entity-to-entity-test-1822000-78.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Entity Field API, +Field API NG blocker

The last submitted patch, convert-test-entity-to-entity-test-1822000-78.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
174.28 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-82.patch, failed testing.

plach’s picture

Issue tags: +Needs reroll

I started looking at this but couldn't finish yet, sorry.

plach’s picture

FileSize
2.37 KB

The problem with the field translation tests seems to be double: to assign multiple field languages to an entity we need it not be language neutral. This is the easy one. The other one is that we are trying to pass an NG entity to _field_invoke() which does not know how to deal with it. It seems we need to use the BC decorator :(

The attached interdiff fixes some failures, didn't look to the others but I suspect they are related.

Edit: as you can see I worked on testFieldInvoke() ;)

Berdir’s picture

Thanks!

Made a few more changes and also updated invoke multiple. Killed the hashing/serializing and just add the entity uuid to the return value. That makes it *much* easier to debug that test.

Also fixed most failures in save/load, just something with default language values not correct?

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-86.patch, failed testing.

Berdir’s picture

Fixed some more translation fails and fixed the translation web test.

Status: Needs review » Needs work
Issue tags: -Needs reroll, -Field API NG blocker

The last submitted patch, convert-test-entity-to-entity-test-1822000-88.patch, failed testing.

plach’s picture

FileSize
3.26 KB

The attached interdiff should fix the latest field translation test failures. However to properly fix them we'd need #1810370: Entity Translation API improvements to go in first, as right now the Entity Translation API does not support multilingual field defaults.

Moreover #2019055: Switch from field-level language fallback to entity-level language fallback is going to completely change how language fallback is applied to entities, so the field display language tests are going to be completely rewritten (or probably just thrown away).

Berdir’s picture

Awesome, thanks!

Here's an updated patch with a major re-roll after all the field array/object changes in the test and also some test fixes.

The tests right now check for the following three things:
1. A non-existing field, is not touched.
2. A field that is set to NULL, values are deleted
3. A field that is an empty array(), values are deleted.

With the check in EntityBCDecorator, I fixed case 2 and 3 but this causes the first to blow up. Does this case even exist anymore, with the defined fields, you can't not have it, can you? So should we simply delete those?

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-91.patch, failed testing.

Berdir’s picture

Fixed some wrong merges and trying to fix sql storage test.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-93.patch, failed testing.

Berdir’s picture

Ok, that __isset() change does weird things.

Removing that, let's try something else, I still think that we no longer support this like that.

Status: Needs review » Needs work
Issue tags: -Entity Field API

The last submitted patch, convert-test-entity-to-entity-test-1822000-95.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Entity Field API

The last submitted patch, convert-test-entity-to-entity-test-1822000-95.patch, failed testing.

plach’s picture

Btw, I really think we should try to make #1810370: Entity Translation API improvements go in asap. It will make completing the field NG transition easier and shouldn't conflict much with the other patches.

Berdir’s picture

6 fails!

Need to look at why the field attach other suddenly fails, it didn't do that before. Taxonomy term index is also weird. Other than those, the remaining ones is the handling of missing fields/incomplete entity objects, which I think we no longer support? And one last translation related fail in field sql storage.

@plach: Yes, following there, will do a review ASAP. Maybe you could have a look at the last translation related fail here in the field sql storage test? I'm not sure if all my changes there make sense...

plach’s picture

Maybe you could have a look at the last translation related fail here in the field sql storage test? I'm not sure if all my changes there make sense...

I will later today :)

fago’s picture

Other than those, the remaining ones is the handling of missing fields/incomplete entity objects, which I think we no longer support?

Indeed - we don't.

plach’s picture

I am looking at the storage failures

plach’s picture

FileSize
2.25 KB

The attached interdiff fixes the field attach storage tests: actually one use case is not supported anymore (fields non defined on the entity object). In the other fix I just realigned the code with original one, which seems broken however. We probably may want to change the assertion and keep the unset instead.

Berdir’s picture

Ups, miscommunication, I fixed the same failures on the plane. My patch is basically doing the same as plach's.

- Completely remove the missing-check from field_attach_update(), that whole thing is going out anyway and we're already in a foreach loop on the field instances there, I was just playing around with that check
- Fix the caching test
- Removed the missing field tests, as discussed, they are no longer supporter, especially after we move to methods on field classes.
- Fix the TermIndexTest, that also did unset() crazyness to avoid re-loading the node o.0.

If those changes don't introduce new failures, I think we're left with ***1*** fail here, "NULL field translation is wiped." in FieldSqlStorageTest, which is the one that I meant in #100.

Berdir’s picture

Issue tags: +sprint, +Field API NG blocker

Tagging.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-105.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
183.08 KB
819 bytes

This should be green.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-108.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
170.57 KB

Quite a re-roll after the field types issue went, also added a few missing files that got lost in the last patch.

A few test methods were removed that we had to fix here, so that patch is a bit smaller, let's see if this works.

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-110.patch, failed testing.

Berdir’s picture

Who thought it would be a good idea to switch the arguments of that function?! ;)

Status: Needs review » Needs work
Issue tags: -sprint, -Entity Field API, -Field API NG blocker

The last submitted patch, convert-test-entity-to-entity-test-1822000-112.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-112.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, convert-test-entity-to-entity-test-1822000-112.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Entity Field API
effulgentsia’s picture

This looks great. Just some minor questions.

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldAttachStorageTest.php
@@ -274,20 +271,13 @@ function testFieldStorageDetailsAlter() {
-   * Tests insert and update with missing or NULL fields.
+   * Tests insert and update with empty or NULL fields.
    */
   function testFieldAttachSaveMissingData() {

Should we rename the method as well?

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldAttachStorageTest.php
@@ -347,152 +328,147 @@ function testFieldAttachSaveMissingData() {
-   * Test insert with missing or NULL fields, with default value.
+   * Test insert with empty or NULL fields, with default value.
    */
   function testFieldAttachSaveMissingDataDefaultValue() {

And this one.

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldUnitTestBase.php
@@ -69,7 +68,7 @@ function createFieldWithInstance($suffix = '', $entity_type = 'test_entity', $bu
-      'bundle' => $bundle,
+      'bundle' => $entity_type,

Why?

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldUnitTestBase.php
@@ -79,7 +78,7 @@ function createFieldWithInstance($suffix = '', $entity_type = 'test_entity', $bu
-    entity_get_form_display('test_entity', 'test_bundle', 'default')
+    entity_get_form_display('entity_test', 'entity_test', 'default')

Since we're changing this anyway, shouldn't it be $entity_type and $bundle?

+++ b/core/modules/field/lib/Drupal/field/Tests/TranslationTest.php
@@ -189,39 +193,45 @@ function testTranslatableFieldSaveLoad() {
+      $values = array('eid' => $id, 'evid' => $revision_id, 'fttype' => $instance['bundle'], 'langcode' => $translation_langcodes[0]);

This seems wrong. Aren't 'eid', 'evid', and 'fttype' no longer the correct key names? Is it a problem that the test isn't failing with these being incorrect?

effulgentsia’s picture

The feedback in #119 is really minor. Once it's addressed, please feel free to RTBC without waiting for me (since I'm 8 hours behind Dublin time).

fago’s picture

The patch looks good to me. So leaving needs-review so that the concerns of #119 can be addressed.

I'm wondering what should happen with these tests in the long run considering #2028403: Mark field_attach_* functions as deprecated (which I just opened). I guess we should either a) remove them if we have already an entity equivalent or b) port it over to the entity system to ensure the same thing is covered there + decide on a test-by-test basis.

plach’s picture

Yes, I already expressed similar concerns to @Berdir. Some tests were already just dropped here just to make the bot happy as we don't support the related use cases anymore.

Berdir’s picture

Thanks, addressed that, also found a left-over form controller to kill.

- Renamed the functions to Empty instead of Missing
- Fixed the bundle, there was only a single case that uses it and that passed in the entity type name. Changed the default value to NULL as the default bundle == passed in entity type.
- Fixed the eid/evid/fttype thing. I don't think it's a problem that it didn't fail, if it's not set then it just uses the default values.

Also just noticed a some unchanged import thats that still references the old entity type but didn't break but it just verifies that the raw files are copied over.

Status: Needs review » Needs work
Issue tags: -Entity Field API

The last submitted patch, convert-test-entity-to-entity-test-1822000-121.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Entity Field API
plach’s picture

Status: Needs review » Reviewed & tested by the community

#119 has been properly addressed and both @fago and @effulgentsia agree this is good. Let's make another step towards removing the Entity BC layer!

plach’s picture

Issue tags: +Avoid commit conflicts
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d0d1817 and pushed to 8.x. Thanks!

plach’s picture

Who-hoo!

Berdir’s picture

Yay!

Wrote a short change notice for this, it was quite common to use the old test_entity's for tests: https://drupal.org/node/2028829

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

jibran’s picture

Issue summary: View changes

We have a remaining @todo in BlockContentFieldTest

/**
 * Tests block fieldability.
 *
 * @group block_content
 * @todo Consider removing this test when https://drupal.org/node/1822000 is
 * fixed.
 */
class BlockContentFieldTest extends BlockContentTestBase {
pcambra’s picture

Asked Berdir about this and completely forgot to create the issue, here it is: #2428633: Remove unnecessary BlockContentFieldTest