Problem/Motivation
At the moment there's no generic way to access the entity language (if available). The need for an API function to rely on for this grows as there are efforts to simplify / unify the handling of translations (e.g. #1282018: Improve UX of language-aware entity forms).
Further the lack of an unified function to fetch this information lead to several implementations of code which tries to determine the entity language (e.g. #1376126: Fix language handling for translatable fields)
Related Issues:
- #1260640: Improve field language API DX
- #1253820: It's impossible to submit no value for a field that has a default value
- #1376126: Fix language handling for translatable fields (#1456186: Interaction of entity, entity_translation & title on entities without a language property.)
- #1282018: Improve UX of language-aware entity forms
Proposed resolution
Introduce a function like entity_label()
which takes care of the language detection for all entity types.
Proposed function signature is entity_language($entity_type, $entity)
.
Logic of the function:
-
Check the entity info for
$info['entity keys']['language callback']
and use it for further processing.
This property is introduced to allow contrib modules to affect the returned language. This new callback functionality will be used by entity_translation to provide the new language-aware entity edit form.
Since this addition is optional existing will fallback greacefully. -
Check the entity info for
$info['entity keys']['language']
and use the defined property to fetch the language information from the entity.
This property is introduced to get around the fact that there's no such thing as a well defined property containing the entity language information. -
Fallback to
NULL
The value returned in D8 byEntity::language()
is LANGUAGE_NOT_SPECIFIED (which in D7 is LANGUAGE_NONE), we cannot use it here to preserve backward compatibility. In fact LANGUAGE_NONE is an assignable value and in D7 we need a way to differentiate between the case where an entity has LANGUAGE_NONE assigned and the one where it has no language property at all. See #12 for details.
Remaining tasks
- Reviewing the patch
- Check which contrib modules need to be changed to be able to use them with the new function
Original report by plach
This has been split from #1260640: Improve field language API DX. See the OP there for a complete summary.
At the moment there is no generic way to access the entity language (if available). We need an API function to rely on for this like entity_label()
. Among the rest this will be useful to fix:
Comment | File | Size | Author |
---|---|---|---|
#47 | entity_language-1495648-47.patch | 826 bytes | plach |
#42 | entity_language-1495648-42.interdiff.do_not_test.patch | 4.83 KB | plach |
#42 | entity_language-1495648-42.patch | 25.03 KB | plach |
#40 | entity_language-1495648-40.interdiff.do_not_test.patch | 1.08 KB | plach |
#40 | entity_language-1495648-40.patch | 25.08 KB | plach |
Comments
Comment #1
plachActually this is targeting D7, for D8 this issue is already being addressed in #1277776: Add generic field/property getters/setters (with optional language support) for entities.
The patch looks big but it's mostly replacing the occurrences of
$node->language
and$comment->language
with calls toentity_language()
.Comment #2
plachComment #4
plachNew attempt
Comment #5
Gábor HojtsyIt would be great to have a more extended description of what this is about. Also, the patch looks like coming out of the blue, but in fact it is based on discussions in the referenced issues where more extended approaches were attempted. This patch would help entity_translation.module in contrib to reuse the node form and other entity forms for translation of the entity.
Comment #5.0
Gábor HojtsyUpdated issue summary.
Comment #5.1
plachUpdated issue summary.
Comment #5.2
plachUpdated issue summary.
Comment #6
das-peter CreditAttribution: das-peter commentedAdded some other related issue to the issue summary.
Patch applied successfully and everything seems to work as expected.
And on my visual review I found not a thing I could complain about ;)
Comment #6.0
das-peter CreditAttribution: das-peter commentedUpdated issue summary.
Comment #7
plachSo, RTBC? ;)
Comment #7.0
plachUpdated summary
Comment #8
das-peter CreditAttribution: das-peter commentedFrom my perspective this is RTBC, but as I don't think I've the in-depth knowledge to mark this as RTBC, I preferred to follow Gabors hint instead and updated the issue summary. I hope this makes the review easier for others.
Maybe we could ask fago for his thoughts, I'm pretty sure the entity API module will adapt this construct.
Comment #9
bojanz CreditAttribution: bojanz commented+1 from me.
Comment #10
Gábor HojtsyLooks good. Has great summary for committers too :)
Comment #11
fagoHm, actually - why? Could you please elaborate on the different meaning between NULL and LANGUAGE_NONE here? To me, that's totally unclear.
This changes/fixes term url aliases to be saved language specific once we have a term language. Might deserve its own issue, but anyway it makes sense. We can also keep it here, but at least it might be worth a mention.
It does not return the language (object), but the language code. This should be visible from reading the function docs, but isn't.
Afaik we have no implementation requiring that, but moreover we should not facilitate the use of such a callback as it won't work with queries at all. Thus, let's better don't add support for it in the first place.
Update: Noted that it's planned for entity translation to use it to temporarily change the entity language. If doing so, why not just change $entity->language? In both ways it's a ugly hack anyway. ;)
This looks like an API change to me - as the default-call has changed. Actually, it looks to me as the default value of the language parameter of field_attach_form() is broken. But instead of fixing it, this patch puts the burden on entity-providing modules to fix the default theirselves in order to avoid an API change.
So maybe, it would be better to do the API-change and not put the burden on entity-type providing modules? That should fix default handling for them automatically and so create less headaches for module developers.
Is that cast really necessary? It shouldn't be so.
Also, the doesn't the patch add support for the user account's language code as entity language? That basically makes $user->language the user's preferring language only, while in the d8 the default is imho to be both. Is there an implication for the upgrade path?
If not, at least we might want to document some where why we are not adding $user->language as entity language.
Comment #12
plachExactly. The main problem here is that the proper default language for
field_attach_form()
is the entity language. Unfortunately in D7 most core entities do not have a language property, so we decided to default to the default language since we did not have a reliable entity language available.Defaulting to the value returned by
entity_language()
insidefield_attach_form()
is a potentially dangerous API change. AAMOF if an entity is not providing the correct language value tofield_attach_form()
, this change would make the field widgets result as blanked when editing an existing entity (see the comment example below).Since LANGUAGE_NONE is an assignable value we need a way to differentiate between the case where an entity has LANGUAGE_NONE assigned and the one where it has no language property at all.
Hence IMO at this point of D7 lifecycle we cannot perform such an impacting API change. This needs to be a purely additive one. For this reason we cannot default to the entity language inside
field_attach_form()
and, yes, put the burden on the shoulder of modules defining entities. Moreover if we returned LANGUAGE_NONE when there is no property available we would have no way to preserve backward compatibilty when passing the result ofentity_language()
tofield_attach_form()
.I realize that we could simply skip passing this value to core entities not having language support, e.g. taxonomy terms, but this way Entity translation would not have a way to make them translatable.
Agreed, I added a note about this. Actually this is not a harmful change since there is no way to assign a language to terms before this patch. Hence only sites adopting code providing language support for terms (likely only ET) will be affected and this will have to be a conscious choice.
I agree it's an ugly hack but the language callback allows ET to implement it in a "clean" way, changing the entity property is way uglier and more error prone. Moreover It would force ET to backup the value to keep track of the actual entity language. Honestly I think the language callback should not be needed in most situations and hence modules taking advantage of it should know the implications of doing so. ET should be able to confine its usage to the entity edit/submit workflow, thus hopefully not affecting queries based on the language property. Moreover label should have the same problem but we allow a label callback, so another reason to keep the language callback is consistency.
Indeed. I reverted this hunk as I realized that comment field language is totally broken and will need to be properly fixed in a dedicated issue :(
Currently the comment form is always in the default language instead of matching the comment language, which is the right way to do things (as nodes show). Probably this has not been spotted before because comment body has always been untranslatable by default and translating comments is surely not a common use case.
Why not?
$form['#term']
is an array and all the functions having an entity-type/entity couple in their signature assume an entity object. AAMOF I added it as I was getting notices.Not sure about this. In D7
$user->language
is used only for the preferred language so I didn't think it was correct to define it as the entity language. I added a comment about this inuser_entity_info()
.Comment #13
plachAnd here is the comment language issue: #1534674: Comment field language is completely broken.
Comment #14
plachTypo. Replaced comment with:
-14 days to next Drupal core point release.
Comment #16
plachnewlines...
Comment #17
bojanz CreditAttribution: bojanz commentedAs far as I can see, fago's feedback was addressed. Back to RTBC.
Comment #17.0
bojanz CreditAttribution: bojanz commentedAdded commerce integration issue.
Comment #18
plachJust to be sure @webchick sees this on the next commit round.
Comment #19
plachAlso increasing priority since lots of stuff are held back by this one.
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedLooks like a great patch overall, but a couple things:
@fago asked for further elaboration on this and got an excellent response here in the issue queue; can we add some of that to the comment too? :)
Also, putting on my (very new) core maintainer hat, I'm pretty scared of the "language callback" property. Without it, this patch is a really clean-looking improvement and bug fix. With it, suddenly it's the case that any module which directly accesses something like $node->language is now doing it "wrong". True, the difference might only matter when certain specific contrib modules are installed, but that won't stop bug reports being filed about it, and this would still mean Drupal core is introducing an API change.
So I'm wondering if we can talk a bit more about the consequences of that. I skimmed through #1282018: Improve UX of language-aware entity forms and got a sense of how it's being used, but what I'm not sure I understood is exactly what requires it. In other words, if we added the new API here but nothing else, what are the places in core and contrib where things like $node->language would need to change to entity_language('node', $node) to allow the new functionality to work? Maybe we can discuss a little more what the potential workarounds for those are instead.
Finally: This issue was cross-linked to #1277776: Add generic field/property getters/setters (with optional language support) for entities (for Drupal 8) a while ago, but the patch there was committed only recently. Can someone confirm that all the functionality being introduced here really does match what wound up going into Drupal 8 already? It looks like there is a standard language property for entities now, but I wasn't immediately sure about "language callback". What's the equivalent method in Drupal 8 for dynamically altering an entity's language (and if there isn't one, why do we need it in Drupal 7 but not Drupal 8)?
Comment #21
Gábor HojtsyDavid: thanks for the review. For the language callback, we can get this patch in without the language callback but frankly, that was the driver for getting this in. The way it is being used is for the entity form to be able to be displayed for different language version of the entity. That was not supported before this patch. I don't think we'd need any other areas in Drupal contrib updated for this to be working well for entity translation, except maybe field modules that had no regard for node language (which are already buggy). Since fields support languages all on the same level (regardless of whether a field value was originally entered on the node or on a translation, those should work with the updated entity form). @plach has this working in the entity translation module patch in context.
Comment #22
plach@David_Rothstein:
The attached patch implements the suggestions from #20
After this patch any module accessing
$node->language
would be doing it wrong even if we didn't introduce the language callback. However nothing is going to stop working by the simple intoduction of theentity_language()
function, since its behavior is fully backward compatible. Only modules implementing the language callback would have a chance to change the way things work, but any module implementing any core hook/callback has the capability to break/alter the core behavior. I don't see how this could be classified as an API change.In core the places that would need to change are the ones touched by this patch :) Namely passing to the various
field_attach_form()
calls the result ofentity_language()
as$langcode
parameter. The other core places that would need to change are the ones dealing with path aliases: the language callback allows to make those portions of code multilingual-aware by changing the entity language to the active form language. This is basically what happens also in contrib: everywhere I have a piece of code that is language-aware (as path aliases are), but that in the case of a node (entity) form would be confined to a single language per node (entity), the language callback allows to magically make it multilingual-capable with almost zero effort on the contrib module side. See #1155132-18: Add Entity Translation support to Pathauto for a typical example. See also #12 for some other detail on this.Getting the same level of support for free is nearly impossible without the language callback. Probably simply making an entity form multilingual-aware would be very difficult, since we would need to manually change all the language codes appearing in it after any form manipulation by
field_attach_form()
has happened. And we would require any module performing a custom addition to explicitly take into cosideration that the form might have a language associated to it. Moreover there would be no official place where taking this language from. This looks like a nightmare scenario to me and something I'm not willing to spend a minute to code for.D8 defines an
Entity::language()
method that basically makes any entity language aware. This coupled with the planned form language (see #1498874: Provide language awareness to entity forms (introduce the form language concept)) provides the same functionaly we are trying to introduce here, but way more cleanly. See also #1188388: Entity translation UI in core for a more general overview.To sum up I don't think the language callback will be a problem if people make a sensible use of it, as ET is planning to do. Rejecting it would kill the work that has been done in the past 6/7 months on the Entity translation project based on a discussion held with @Gabor and @webchick, as Angie assured me she would commit the patch in that precise moment had it been ready. I really hope this is not the direction we are going to take since I'm pretty sure it would not be the best choice for the future of Drupal multilingual support.
Comment #23
plachSurely this is an API addition, I thought this was already tagged as such.
Comment #24
andypostthere's a mention about forms in #1448330-25: [META] Discuss internationalization of configuration
Also I think the langcode properties/fields affects mostly on render layer of core
Comment #25
Gábor Hojtsy@andypost: how would that relate to Drupal 7?
Comment #26
plach@andypost:
I don't get your point: are you sure you are on the right issue?
Comment #27
plachSetting back to RTBC to ensure this does not get out of @David's radar.
Comment #28
plachComment #29
David_Rothstein CreditAttribution: David_Rothstein commentedDon't worry, I have no intention of rejecting something that has been carefully developed for 6-7 months :) I'm just trying to make sure we're aware of the consequences of what we're doing here and have minimized any negative repercussions. Hopefully you can appreciate why this issue is a particularly tricky one; while I guess it's not quite an API change, neither is it really a standard "API addition" (since it requires cooperation from other code in order for the new API to be used). That's why I wanted to be extra sure about what other kind of code needs to change for this to work; thanks for providing that information.
Can you clarify that statement? With the exception of the language callback, I don't see how $node->language is different from entity_language('node', $node). (Unless someone wanted to try altering the 'language' key itself via hook_entity_info_alter(), but I don't think that's something that could ever work anyway.)
I've been thinking about this a little more, and one of the reasons I'm feeling better about this patch is that it actually seems very similar to the situation we currently have with entity_uri() and the 'uri callback' property. We still have code that calls "node/$nid" directly and we don't consider that a bug (and in some places it never could be converted even if we wanted to because the code doesn't have the correct context). I believe the semi-conclusion that was reached on that topic e.g. in #823428: Impossible to alter node URLs efficiently is that if modules want to try altering the standard 'uri callback', they shouldn't necessarily expect it to work in all scenarios and for all entities, but it's really more of a best effort thing designed for particular uses cases.
Given that this situation is similar and especially given that we're years past the Drupal 7 code freeze, here's what I think we should do here:
I think those changes will help set expectations about how these new APIs can be used.
Finally, I'm still a little unclear on the equivalent for 'language callback' in D8. Yes, #1498874: Provide language awareness to entity forms (introduce the form language concept) might help with that, but that's not in D8 yet. Also, it's the main use case this is being introduced for, but once 'language callback' exists in D7 people might start using it for other things too. I want to make sure we avoid adding a feature to D7 which disappears on people in D8. So if there is no equivalent for being able to hook in and dynamically alter an entity language in D8 currently (and I can't think of one) I think the simplest thing to do would just be to add it there also? Even though the entity system has been refactored since D7, D8 still has the 'uri callback' property, and again, this would be similar.
Comment #30
David_Rothstein CreditAttribution: David_Rothstein commentedSetting back to "needs review" to discuss those remaining points.
Comment #31
plachSorry for not being more explicit about this: as I was writing in #12, the model behind this patch actually is the
entity_label()
function, which, as you are pointing out, is an opt-in API function. However, leaving alone alterations to the entity keys, theentity_(uri|label|language)
functions should not be optional, as they are a primitive form of encapsulation of the logic needed to return their respective information. The only reason to directly access the entity information is that in D7 we have no OOP and visibility modifiers to enforce encapsulation. When I say that after this patch lands accessing$node->language
is wrong anyway, I mean that it is conceptually wrong even if things keep working exactly as before. From this POV, you are right, this is more than an API addition in the sense that suddenly perfectly legal code becomes not so legal :). However nothing should break and this is what matters here.Exactly. Hopefully many will use the
entity_language()
function but very few will mess with the language callback.TBH I don't see the value of adding an API function and then saying it's not required, although you'd better to. If every module maintainer replaced all the usages of
$node->language
with calls toentity_language()
the day after this is commited my life would be way easier :)Also there is no such recommendation for
entity_label()
, despite it never being used in core. If your concern is that, given the time this API addition comes up, people might be unconfortable with adopting it, perhaps we may want to provide the additional information you are suggesting above in the change notice?You are right, this would have no strict equivalent in D8 atm. Personally I'm not totally sold on the value of those callbacks (uri, label) once the information they relate to is properly encapsulated (a subclass or a different interface implementation might provide the same functionality in a cleaner way). Rather I'd easily see them go away in future refinements. I, for one, would totally support the language callback to be dropped in D8. Not sure how to proceed here.
However, I made a small improvement to docs while we discuss further integrations.
Comment #32
tim.plunkettFixing tag.
Comment #33
plachSetting back to RTBC to ensure @David does not miss my reply.
Comment #34
plachComment #35
David_Rothstein CreditAttribution: David_Rothstein commentedGábor/others: Any thoughts on how to proceed here?
I don't feel too strongly about changing the entity_language() documentation, since probably anyone who finds the function in the first place should just go ahead and use it... So I'm fine with mentioning its usage in the change notification and/or release notes instead.
I do still think the limitations of 'language callback' should be documented directly in the patch though; it just seems like a good way to be honest with people who might want to use it or rely on it, especially with the whole thing being a new API whose uptake we can't really be sure of. (I think we should do the same for 'label callback' and 'uri callback' for what it's worth, although that's a different issue.)
So the big question remaining is the D8 equivalent. Let's see, if everyone in D8 creates new entities using entity_create() (which I suppose they should), then maybe it's true that subclassing an entity type and overriding the language() method does provide equivalent functionality... because if your module also implements hook_entity_info_alter() and uses that to alter the entity's class, then entity_create(), entity_load(), etc. should always respect that. So perhaps that's a good enough equivalent after all?
Comment #36
Gábor Hojtsy@David: Yes, we do expect all entities to be OOP-ified in Drupal 8, not everything is there yet, but it is a work in progress. Such as #1361226: Make the file entity a classed object that just got recently RTBC-ed again.
Comment #37
fagoI'd agree with that - I see no need for the callback in d8. Language needs to be query-able. So, if we introduce it now, we should probably mark it as deprecated?
Still, we are in fact introducing this for ET only or are there any other use cases as well?
If not, I'd tend to have ET stay with the entity property change. Backing up the real-language shouldn't be hard (e.g. use an #entity_builder) and sounds less error-prone to me than the language callback variant.
Problem of the language callback variant: As a module developer I'd expect the entity type has a not query-able language given the callback is used, so I'd have to disable any functionality depending on a queryable language for this entity type.
Comment #38
plach@David:
I added some additional documentation providing infromation about the relative unreliability of the language callback.
@fago:
Well, it's no secret I came up with this solution having ET in mind but, as I hinted in the docs, the language callback may be useful to provide language support to entity types not having it from other modules than the defining ones.
Backup is not my primary concern, actually I'm more worried about not being able to persistently change the value for the whole request processing and that the replaced value might be stored to the DB by contrib/custom code. However if there is no alternative I'll try to go this way. I hate this idea though, as it will further delay a "stable" release of ET.
This makes totally sense but, given the 'volatile' nature of the value returned by the language callback, what about suggesting that the entity is still queriable by language if it defines a language property? This would serve as the main entity language source albeit being alterable (see the attached interdiff).
I still have the
entity_label()
parallel in mind: if I define a label callback for each available entity type to translate their labels on rendering, I don't expect those types to become suddenly non-queriable by label.Comment #39
David_Rothstein CreditAttribution: David_Rothstein commentedThat kind of thing is one of the reasons I wanted the documentation added here - we definitely don't want to give people that impression. Do you think the new documentation added in the above patch is sufficient to communicate that? It looks pretty good to me.
Not sure if there's a typo above or it was just unclear to me, but "entity-defining modules are encouraged to always rely on it".. makes it sound like "it" refers to the language callback property. Perhaps change to "entity-defining modules are encouraged to always rely on the language property" to make it clear what is being referred to.
Comment #40
plachFixed docs as per #39.
Comment #41
fagoThat confuses me? opt-in API? :) I guess it's more worth documenting it for the label callback, not for the API function. The API function is save to call.
For adding language support, I think the preffered way should be to alter in a proper language column using hook_schema_alter().
Again, this confuses me. So if I provide an entity type, I should not use this API function? Or not use use the callback?
It's fun making todo's that are already done :)
Should we make a change record for the usual invocation of field_attach_form() being changed?
Also, could we please properly communicate the taxonomy path language changes as well?
Comment #42
plach@fago:
Agreed, moved this sentence to the language callback docs.
I didn't mention this possibility since in the past messing with core table caused upgrade path troubles. However I removed the related doc lines.
I tried to say that when you define a new entity you'd better always define a corresponding language property, since relying only on the callback has implications on the entity queriability. Hopefully now docs are more clear on this.
Yeah, I hoped this would get in far earlier ;) Slightly adjusted the docs here too.
Do you mean warning people that entity_language() is the new preferred default for the langcode parameter of field_attach_form()? Yes, I definitely think we should.
Well, actually nothing would change here unless one enables a module implementing the language callback for taxonomy. So this is only a future possibility. However it might make sense to provide a note about this too.
Comment #43
fagoIf I'm not mistaken this allows for language-specific path alias for terms?
ok, so we'll need two change then? One for entity_language() got introduced and one for field_attach_form() should be used differently now?
@improvements: Yep, that's better now thanks. The language callback docs seem to be a bit verbose though, but better verbose docs as too less. Then, I still dislike the language callback, but that has been discussed extensively in that issue anyways and it seems to be the best way forward now. So back to RTBC.
Comment #44
plachYes, only through the installation of a contrib module implementing the language callback though. The core code keeps creating language neutral aliases for taxonomy terms. This opens new possibilities but there is no actual change in the current behavior to notify.
Not sure about the best practices here: maybe @David can enlighten us?
A huge thank you for this. I hope you won't regret for not being stronger about it, but honestly I feel we already have too much hacks in the entity translation workflow to introduce yet another tricky one. That said, I don't like this solution too and I wish I realized during the D7 API polishing phase that a function like entity_language() could still be introduced. This would save us many headaches but, as you know better than anyone else, all the D7 core entity API is half-baked and this is yet another attempt to make it less incomplete.
Thanks again :)
Comment #45
David_Rothstein CreditAttribution: David_Rothstein commentedThanks for your patience on this issue. I think it looks good now, and I'll leave it for another day or two in case anyone else wants to review it, but after that I'm intending to commit it.
Regarding the change notification, I would say it makes sense to combine the entity_language() and field_attach_form() notifications into one. (Basically, they both fall under the category of "things that you're recommended to do if you want your module's code to support translations better" so it's best to summarize that all at once.)
Actually, though, I think we still will need two change notifications:
Comment #46
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x and added to CHANGELOG.txt - thanks! http://drupalcode.org/project/drupal.git/commit/a121523
Moving back to active for the change notifications.
One small question for possible followup:
Should this be changed to have a sentence like "This entry can be omitted if..."? The other optional entity keys in hook_entity_info() have sentences like that, and not all entities actually support a language property (or even make sense to).
Comment #47
plachWho-hoo!
And here are the change records:
http://drupal.org/node/1626346
http://drupal.org/node/1626352
Comment #48
plachUnblocking the major queue.
Comment #49
fagothanks!
I've edited [#1626352] to simplify the last part of the sentence a bit. [#1626346] gives a good summary of the whole issue, but lacks a really short overview. Could we add something in the lines of:
"A generic way to access the entity language (if available) has been introduced: now it is possible to use the entity_language() function to retrieve an entity's language, if the entity type defines it. In order to support entity translation in your entity forms, make use of it when calling field_attach_form() as following:
Done, that's all someone really needs to know. Continue reading the details if you care. Makes sense?
Comment #50
fagoComment #51
plachWell, that summary is very specific to the entity translation part, what about moving the last paragraph ("This is a full backward-compatible API change [...]") on top after the very first one? Changes to the other CR look good.
Comment #52
fagoYep, that would be a good idea as well. Still, I think we should include the field_attach_form() before/after comparison in the "overview", so you don't have to go through all details in order to know what to do.
Comment #53
plachUpdated the CR moving up the last paragraph and adding the text from #49.
@fago:
What about the small follow-up patch?
Comment #54
plachTentatively marking RTBC, since the patch is pretty staightforward.
Comment #55
webchick#47 seems to be what David was asking for, so committed/pushed to 7.x. Thanks!
Comment #56
plachThanks!
Restoring the original priority.
Comment #57
David_Rothstein CreditAttribution: David_Rothstein commentedYup, looks great; thanks for the quick turnaround.
I also went ahead now and linked to the change notification in CHANGELOG.txt: http://drupalcode.org/project/drupal.git/commit/d072581
Comment #58
plachFYI the D8 counterpart of the default for
field_attach_form()
is being addressed in #1499596: Introduce a basic entity form controller.Comment #59
Gábor HojtsyDone, can move off of sprint now.
Comment #61
hass CreditAttribution: hass commentedMy module uses
$node->language
and I'd like to convert them, but how can this backward compatible? Theentity_language()
function is only available in Drupal 7.15+ and not in 7.14 and before. This means it is not backward compatible and if a module implements the new function, it forces everyone to upgrade to 7.15 or later. Otherwise the users may see messages like function does not exits and so on. How can I prevent this?Should this solved this way?
dependencies[] = system (>7.14)
Comment #62
tim.plunkettUse function_exists('entity_language'), or make your module depend on 7.15 like you suggest
Comment #63
hass CreditAttribution: hass commentedI cannot add everywhere function exists calls!
Comment #64
hass CreditAttribution: hass commentedReopening as http://drupal.org/node/1626346 needs more information like
dependencies[] = system (>7.14)
.Comment #65
bforchhammer CreditAttribution: bforchhammer commentedYou can avoid the dependency by using a "compatibility layer" such as the following:
Comment #66
webchickThis issue is fixed, if you have a support request about how to specify minimum dependencies in your module, please open a separate issue for that.
Comment #67
hass CreditAttribution: hass commentedI'm only saying that there is a major DX issue. Saying it is backward compatible is plain wrong and this need to be corrected in http://drupal.org/node/1626346
Comment #68
hass CreditAttribution: hass commentedThe update notice needs changes to be clear. I would have replaced it without thinking more about. It's plain wrong that I should just replace all $node->language with entity_language().
Comment #69
webchickThose are just wiki pages. Please feel free to edit it however makes it clear.
Comment #70
plachI added some notes to the change notice as suggested by @hass. Hope it looks better now.
Comment #71
hass CreditAttribution: hass commentedThanks for adding these. I'm going with the system dependency key for now as it looks cleaner to me. Hopefully not so many people are running old versions.
The code example in #65 looks better than the snipped added to the docs as the example only returns LANGUAGE_NONE where we have mostly a language in the entity (pre 7.15). I would suggest to change the example code. I'm not mr. entity, so I'm not changing any articles about these topics myself as I could be wrong.
Comment #73
hass CreditAttribution: hass commented@bforchhammer: Shouldn't it not this? In
entity_language()
function it's noted that returningLANGUAGE_NONE
may introduce DC issues.Comment #74
plachThis code will interpret
$user->language
as the entity language, which is not correct.Comment #75
hass CreditAttribution: hass commentedIt's $node->language not $user->language. It's the way how we detected the node language before we had 'entity_language' function... the only think strange in pathauto is LANGUAGE_NONE that is documented in entity_language to break something - I have not understood.
Comment #76
plachThe following code:
might not always work as intended because
$user->language
holds the user preferred language which may not be the same of the user entity language. We had this issue in Pathauto. If you wish to use this code in the change notice you should document this possibility and retain the complete function which had a flag to skip checking the language property. Btw, this is why I originally used a simpler example.Comment #77
hass CreditAttribution: hass commentedWell I have not understood the $check_language_property in pathauto and it is not properly documented why it's there. Now I have more question marks in mind... can you provide examples when a $user->language is not the same like an user entity language or what's the difference? I just thought I have finally understood what an entity is, but I start to doubt about this again. I'd really like to understand this as I planed to implement user fields support in linkchecker and I do not like to run into unknown troubles.
Should we add a $entity->type == 'user' condition to make it easier or is this really conditional per individual request? This may get answered by the above answer. :-)
Comment #77.0
hass CreditAttribution: hass commentedUpdated the issue summary as the D8 issue has been committed before this one.
Comment #78
fagoActually, no we miss the language key in Drupal 8. See #2143729: Entity definitions miss a language entity key
We don't need the language callback though, as it was only introduced (late) in Drupal 7 for entity translation for being able to hack-around core APIs - which is not necessary for d8 any more.