Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
See #1744302-13: [meta] Resolve known performance regressions in Drupal 8. Postponed on the remaining fieldable entity types not yet converted to EntityNG: #1818570: Convert users to the new Entity Field API and #1983548: Convert contact message entities to the new Entity Field API.
Comment | File | Size | Author |
---|---|---|---|
#82 | entity-bc-removal-1983554-82.patch | 72.51 KB | effulgentsia |
#80 | entity-bc-removal-1983554-80.interdiff.txt | 933 bytes | plach |
#80 | entity-bc-removal-1983554-80.patch | 72.54 KB | plach |
#72 | entity-bc-removal-1983554-72.patch | 72.34 KB | effulgentsia |
#72 | interdiff.txt | 4.1 KB | effulgentsia |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedtagging
Comment #2
webchickI don't see how we can ship with both, so raising to critical.
Happy to see this issue, though. :D
Comment #4
webchickHm. Looks like both dependent issues have been resolved, can we do this now? (Or was it already done in the past 8 weeks and I missed it? :D)
Comment #5
catchIt wasn't done in the past eight weeks. I'm pretty sure there's dependent issues not listed in the summary (modules still using the bc layer for nodes, field items, config entities?) that aren't fully done yet.
Comment #6
plachWe need at least to complete the conversion of nodes and users and the field storage stuff.
Comment #7
BerdirYes, there are some things left.
* Remove BC from nodes: #1939994: Complete conversion of nodes to the new Entity Field API
* Remove BC from users: #2017207: [meta] Complete conversion of users to Entity Field API
* Convert menu_link #1842858: [PP-1] Convert menu links to the new Entity Field API
* Convert field storage code to use NG, first step #1497374: Switch from Field-based storage to Entity-based storage, then follow-up to update it to use the new Code, possibly together with merging the NG database storage controller together.
Those issues are already all critical on their own. What's not yet completely clear is what will happen to config entities. But I currently don't really see a solution make them NG, so we might move the complex data interface down to content entity interface and merge EntityNG into ContentEntityBase. #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface is a first step towards moving interfaces/methods down to ContentEntityInterface/Base.
There's some clean-up stuff to do then, the new Field Type stuff still supports BC entities, but that is no longer necessary, merge some NG controllers with their non-NG pendant or rename to a Content-version.
Comment #8
webchickThis will obviously be a BC break, but we need to do this before releasing. Tagging.
Comment #9
xjm@webchick added these tags in #8, but it looks like Tag Monster is out in force today.
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedAlso opened #2074697: Convert EntityCacheTest and FieldUITestNoBundle to EntityNG
Comment #11
BerdirNinja-style removal of the BC decorator ;)
This obviously breaks any test that will attempt to save or load field values.
Additionally, the following allows to load field item values, but only for the default language. With that, I'm able to look at a rendered node, so it might be easier to the remove BC deorator than I thought. But there will for sure be special cases that I'm not seeing yet ;)
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedCurious if we can at least get this far (only use BC for storage hooks).
Comment #15
BerdirInteresting. This probably doesn't explode as config entities (have to) implement IteratorAggregable but getIterator() returns an empty iterator, so this never returns anything. Because they would return raw scalar values, which would obviously explode.
That's a pretty crazy thing to rely on, so it probably makes sense to get #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface in first, as that moves the validate() stuff down to ContentEntityFormController.
And nice, not too many fails remaining.
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedJust a reroll. Still need to fix the failures and address #15.
Comment #18
BerdirTagging for rocketship focus
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedThis fixes the failures, but doesn't yet address #15.
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedStill just trying to get a green patch here before addressing #15. The addition of getBCEntity() to field_language() in #19 wasn't aggressive enough, causing the 2 new failures. This makes it more aggressive. Would be nice to fix that whole function's pipeline to work with EntityNG, but I don't know how to.
Comment #22
BerdirRe-roll and started with field storage. Field language handling is not correct yet, but it seems to be able to storage and load stuff at least for single-language entities.
Comment #24
BerdirYeah, assumed something like that :)
- The file usage stuff is I think dealt with in #2015697: Convert field type to typed data plugin for file and image modules, so let's get that in first.
- field cache seems to be a bit strange, maybe we need to change that to only cache values?
- The language stuff might require #2019055: Switch from field-level language fallback to entity-level language fallback, not sure. Needs @plach I guess.
Haven't fixed any tests, but the attached patch removes all BC related code that I could find. Interdiff isn't very useful, mostly just removed code.
The @todo in EntityNG::__get() is a bit unfortunate, there's code that now relies on this, e.g. render controller do things like $entity->content['bla'] = blub. Not sure if we want to break that.
Comment #26
yched CreditAttribution: yched commentedYay !
Is the (array) cast still needed ? Field::getValue() can return NULL ?
I'm very likely missing something, but this looks weird, you don't fetch $entity->getTranslation($entity, $default_langcode) ?
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedRe #26.2, that test is weird to begin with: it's testing a content_translation functionality of a config entity. The change to that test is to make it effectively the same as what HEAD is doing in the base class for non-NG, now that the base class removed that check. I wonder if we want to remove that test entirely in #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface, or figure out what its real purpose is.
Comment #28
plachThis should fix some failures: field cache was broken wrt field language, it was storing just one language. I tried also to provide a quick fix for
field_language()
, the actual fix is #2019055: Switch from field-level language fallback to entity-level language fallback.@effulgentsia:
IIRC that test was introduced right after committing the first Content Translation patch, because it was erroneously trying to store translation metadata also for config entities. With #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface CT will officially support only content entities but it might make sense to keep that test to check that CT is not trying to support stuff it cannot. We can remove that test once #1916790: Convert translation metadata into regular entity fields is in, since the original bug will no longer be reproducible.
Comment #30
BerdirThanks, getting there!
The EntityResolverTest is ugly, that's an endless loop because the entity apparently references itself and getValue(TRUE) there causes it to be loaded again and again. And we don't want to cache the computed entity referenced, but we do want to cache the computed processed texts for example.
Maybe introduce a new method on Field(Item)Interface, something like getCache() or getCacheValues() ? And introduce a new cacheable key for property definitions?
Comment #31
plachWhat about implementing
Serializable
in the entity reference field?Edit: scratch that, probably it would quickly become more complex than going with #30
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedI actually agree with the initial suggestion in #31. Rather than introducing a new made up method name, like getCacheValue(), I think Field(Item)Interface should extend Serializable.
Comment #33
effulgentsia CreditAttribution: effulgentsia commentedOops. I think I just went through the same process that plach did when he reconsidered #31. Serializable must return a string, so no, that's not what we want. getCacheableValue() makes sense then.
This should be
$entity->getTranslation($langcode)->$field_name->setValue($items);
, then, right? Or do we need a separate method, like setCacheableValue()?Comment #34
Berdir$field_name = $items
calls __set(), which in turn calls setValue(), so either one works.Serializable is actually broken/weird with 5.4/5.3, so it would have to be __sleep(). That it's a string isn't that relevant, the bigger problem is that after unserializing it again, it's again a field object, and when you do $field_name = $items, it still calls setValue(). That checks for typed data interface and calls getValue(), without TRUE, which means without computed properties.
So we'd have to call getValue(TRUE) or getCacheableValue() on the object we get back from the cache.. which would be useless, we'd better just store that in the first place :)
Comment #35
yched CreditAttribution: yched commentedYes, I don't think we want to store serialized FieldField/Item objects in the cache (and definitely not including full serialized referenced entities ?). Wouldn't it kill the optimization of not creating those objects on a simple entity load ?
Does text 'processed' really have to be a 'computed' property ?
We'd need to make sure it's always present and to-date with the raw text values, but if it was a regular property autopopulated on load / read from cache / value change, then maybe we could stick with "cached values do not include computed properties" ?
Comment #36
plachThis should fix comment translation tests and entity translation API tests. Actually the root cause for both failures were translations not being properly handled during object cloning. Somehow the BC decorator was masking this. I also needed to fix more stuff with language fallback: hopefully I didn't break anything, otherwise it would probably better to get #2019055: Switch from field-level language fallback to entity-level language fallback in first.
This patch also adds a small optimization for field cache handling by instantiating the translation objects fewer times.
Comment #38
plachThis should fix more failures. We should be left only with file-related failures which should be fixed by #2015697: Convert field type to typed data plugin for file and image modules and the entity reference cache issue above.
Comment #39
plachBerdir suggested in IRC to address the field cache issue separately. Created #2083785: Add support for determining which field properties are cacheable, although I probably won't have time to work on it.
This is now blocked on that one and #2015697: Convert field type to typed data plugin for file and image modules.
Comment #41
plachWe should be ready for review as the remaining failures should be fixed by the issues above.
Comment #42
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll for #2015697: Convert field type to typed data plugin for file and image modules landing.
Comment #44
plachFixed the new failure.
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedAnd just for fun, let's see if anything fails if we simply decide to not cache the processed text, in order to not hold this issue up on #2083785: Add support for determining which field properties are cacheable.
Comment #46
effulgentsia CreditAttribution: effulgentsia commentedThis addresses #26 (incorporating the reply of #28), and some lines I found cumbersome from field.multilingual.inc.
I reviewed the rest of the patch, and it all looks great to me. I'd be comfortable with this being RTBC'd so long as someone approves the #45 interdiff and this one.
Comment #48
effulgentsia CreditAttribution: effulgentsia commentedI guess the answer to #26.1 is yes.
Comment #50
plachI'd revert the change to field.multilingual.inc: since we are trying to get rid of it altogether in #2067079: Remove the Field Language API, I'd avoid adding more functions to it. Otherwise we'll have to reroll #2019055: Switch from field-level language fallback to entity-level language fallback to remove
_field_translated_value_exists()
right after this is committed.Comment #51
yched CreditAttribution: yched commentedWhy do we need to remove this condition ?
The list of languages returned by field_available_languages() might vary over time - e.g. if the field changes from translatable to untranslatable, + that list has an alter hook currently, + more generally I'm not sure what happens when a language gets removed from the system...
This condition ensures that we only populate the entity with languages that are valid for the current runtime context.
Comment #52
plachYeah, right, we should limit the query to the available entity translations.
Comment #53
plachThis restores the previous behavior with the exception of
hook_field_available_languages_alter()
, which I don't think we should support anymore, as I was saying in #2067079: Remove the Field Language API. We should be ready if this is green.Comment #55
plachLet's try this.
Comment #57
plach#55: entity-bc-removal-1983554-55.patch queued for re-testing.
Comment #59
plachThis should restore the field storage language behavior presently implemented in HEAD. Revising it is being discussed in #2076445: Make sure language codes for original field values always match entity language regardless of field translatability.
Comment #60
effulgentsia CreditAttribution: effulgentsia commentedIn HEAD, field_available_languages() also checks that the entity type is translatable, but the new code does not. Why?
Comment #62
plachRerolled
@effulgentsia:
Actually it is checking whether there is any translation handler, which is a deprecated concept. Currently we have a BC layer in place which falls back to checking whether the entity type is translatable (the closest thing we could come up with in D8), but that is going away soon. That check is now superfluous as a field cannot be made translatable if the related entity is not translatable. Even if some module worked around this limitation, field translatability would still be the only thing that matters to determine which languages should be retrieved.
Comment #64
plachFixed field items saving. Hopefully this should be green again.
Comment #65
effulgentsia CreditAttribution: effulgentsia commentedThanks. I'm tempted to RTBC this, but I'll give it 12 hours or so for yched to double check.
Comment #66
BerdirAlmost back from holidays :)
I'm fine with getting this in without the cache for computed properties *but* that means we need to make [#/2083785] critical (and get an API change approval in case we need to change things) because that will introduce a *serious* performance regression, that we already fixed once last week.
I'd also love to see a follow-up issue to improve the entity loading/object creation. If we manage to build up a single $values array consisting of both base and configurable fields and then just call new $class($values), that would be awesome, because then we'd really only needed to create field objects for the values that are used. That was the idea of all that lazy loading stuff but as long as we do $entity->$field = $value, we have to instantiate all that stuff. I guess that will only work when we have an entity cache, no just a configurable field cache. Thinking about that part, could we possibly just generalize that a bit to base fields too? Then we could pretty easily cache all those things too...
I was also surprised to not see the tests fail that I added for the processed caching, but that's because I only tested the cache where the cache already exists, I'm not validating that the cache is written correctly. We need to improve this in the other issue.
Comment #67
yched CreditAttribution: yched commentedWill try to review asap.
Big +1 on that. The current code flow between attachLoad() / mapFromStorageRecords() / attachPropertyData() is very convoluted and makes weird switching between entities as loaded array records and entities as real objects.
And +1 on working on a "base + configurable" fields data cache, the current situation is absurd (all FieldItem classes can have a prepareCache() method, but it's only invoked for configurable fields).
I haven't really looked into how easy it would be, partly because of the above (loading of base field data is confusing right now). I've mentioned that as one of the points to discuss during the pre-conf sprint meetings in Prague.
Comment #68
plach+1 on #66 and #67!
I am already trying to clean that up a little in #2057401: Make the node entity database schema sensible, but we will a lot more :)
Comment #69
yched CreditAttribution: yched commentedDoesn't seem to apply anymoreSorry, my bad
@plach: you shouldn't name your interdiffs "interdiff-something.patch", it messes with drush_iq. interdiff.txt usually works fine ;-)
Comment #70
yched CreditAttribution: yched commentedComment #71
yched CreditAttribution: yched commentedIn doLoadFieldItems() we load values for langcodes array_keys(language_list()) (i.e all languages known by the system)
In doSaveFieldItems() we save values for langcodes array_keys($entity->getTranslationLanguages())
Why don't we also rely on $entity->getTranslationLanguages() on load ?
Is it because ->getTranslationLanguages() returns langcodes based on what is found in the $entity and thus cannot be used while we are populating the entity ? In that case, a comment might be useful.
Personal taste I guess, but I'd rather avoid the "continue" and either switch back to casting to an ampty array, or wrapping the code in an if ($items = ...) {do stuff}
Maybe it's just me, feel free to ignore.
This looks really weird, but no better suggestion on my side, this area is a bit over my head :-/
That's a lot of ':'s ;-)
return $field->isEmpty() ?
Comment #72
effulgentsia CreditAttribution: effulgentsia commentedThis addresses #71's 2, 4, and 5. I don't know what to do about 1 and 3.
Comment #73
webchickJust one observation/question, not a review:
I'm surprised that this class still exists after this patch. I would've expected it to become just "Entity." When/where is that happening? We can't ship D8 with something called "EntityNG" IMO.
Comment #74
BerdirYes, I said a while back that this will happen when we will remove EntityBC. In short, EntityNG as a name will not stay, but it won't be removed in this issue, that will happen in #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface.
To explain, back when I said that, we assumed that all entity types, including config entities will be NG'ified. However, nobody had time to work on it so it didn't happen. Given the number of config entities that we have by now, that would be a crazy effort. But, we no longer need EntityBC, which is only used to access already converted entity types in the old way. EntityBC was never used with config entities. The only remaining database entity that is not converted to EntityNG is menu_link, and that is not fieldable currently. That means we can safely kick the EntityBC code out.
What the referenced issue will do is rename EntityNG to ContentEntityBase (and slightly change the meaning of content entity, also making e.g. users a content entity), which means that a large number of functionality will only work with entity types that implement ContentEntityInterface: having configurable fields, content translation (makes kinda sense ;)), being supported by rest.module (at least the current implementation that we have in core, it should be possible to integrate with config entities specifically too) and edit.module.
That's not perfect, it would be nice if config entities would support the same API, but config entities are very different to content entities in many ways, so I think it's a reasonable compromise, the only one that I see. That's to be discussed in the referenced issue if necessary, just wanted to explain it.
Comment #75
webchickThanks, that's very helpful. I'll follow that issue, too.
Comment #76
effulgentsia CreditAttribution: effulgentsia commentedAny chance of this issue being RTBC'd without addressing #71.1 and .3, and punting those to follow ups?
Comment #77
yched CreditAttribution: yched commentedAgreed, let's do that.
@plach, could you check #71 1. and 3. and see if they deserve followups ?
Comment #78
webchickThis is absolutely impressive work, folks. Thank you SO much!
Tentatively tagging "needs profiling." Not to block a commit, but because I'd love to figure out the performance increase we gain by removing all of that code.
Just a warning that I'm in the process of committing a bunch of stupid and pedantic DX clean-up patches today just to get them out of the way before DrupalCon which might break this one (hope not, but...) So maybe hold off on any re-rolls until tomorrow.
Comment #79
BerdirNot exactly sure how all the translation languages stuff plays together, but I also think it's related to the the actual field values, we need those before we know what translations we have. A comment might make sense but I'm not sure what we could write that would also make sense in the final code and not just explain the change in the patch.
As discussed in IRC, there's not much to profile here. This issue actually introduces a cache related performance regression, so might be slower than HEAD with a good amount of content on a page (due to slow check_markup() calls). So a before/after comparison will have a lot of noise in it and we can't say something like it's now 5% faster. Especially because most calls already have been converted to use EntityNG directly apart from field storage and we're mostly just removing dead code.
The main reason that this is relevant for performance is that it allows to do useful profiling again as this is the final part of removing a lot of magic-on-top-of-magic calls that itself does another calls (e.g., language() used to be called *a lot*), so we can actually look at how we use it and how we can improve it. One obvious example (already pointed out above, needs an issue) is the whole entity loading mess that currently hijacks a lot of the lazy-loading logic that is supposed to speed EntityNG up.
Comment #80
plachThe reason is that we are loading field values for multiple entities, hence we cannot limit the query to specific languages because the available translations might be different for each entity. Added a comment to clarify this.
When cloning a translation object, the
EntityNG::translations
array is a reference (seeEntityNG::initializeTranslation()
). This means that it would be shared between the original object and its clone, which would cause weird behaviors. The only way I found to get an actual clone of the original array is unsetting the reference and setting the translations array back as an actual value afterwards.Comment #81
webchickPatch no longer applies.
Comment #82
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll.
Comment #83
effulgentsia CreditAttribution: effulgentsia commentedRemoving "Needs reroll" tag.
Comment #84
catchCommitted/pushed to 8.x, thanks!
I'm a bit confused which issue fixes the check_markup() caching regression, is it the computed properties one?
Not sure if this needs a change notice, maybe a short one for contrib entities which ought to completely break now if not updated?
Agree with the ContentEntityBase stuff etc. fwiw.
Comment #85
yched CreditAttribution: yched commented@catch: yes, that's #2083785: Add support for determining which field properties are cacheable
Comment #86
effulgentsia CreditAttribution: effulgentsia commentedFWIW, I think that situation is completely ok. In many other systems, "entity" just means something that is persisted and is identifiable. ConfigEntityBase and ContentEntityBase then layer additional capabilities on top of basic CRUD, each one optimized for its use case and storage. Makes total sense, I think. Perhaps some further unification will be possible in D9, but I don't see the lack of that unification in D8 as bad at all.
Comment #87
tstoecklerMinor follow-up: #2090973: Minor comment typo in EntityNG
Comment #88
yched CreditAttribution: yched commentedNote: the changes in DatabaseStorageController::doSaveFieldItems() broke installation of the 'standard' profile in languages other than english - #2091523: Install in foreign language fails with integrity constraint violation on 'user_picture_target_id'
Comment #89
BerdirI don't know what do document here. I don't think we have an change notices about the BC decorator and the change notices that we created already use the new API's.
Unless contrib modules did a manual getBCEntity(), they already didn't see the BC decorator anymore for a while now.
Comment #90
BerdirThose sprint tags..