Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sokarion created an issue. See original summary.

Sokarion’s picture

Sokarion’s picture

Status: Active » Needs review
stefanos.petrakis@gmail.com’s picture

Hey @Sokarion, thanks for this report and apologies for the delay.
I cannot see the error on the screen when creating a node.
Could you give a little more details on how to reproduce this?

stefanos.petrakis@gmail.com’s picture

Status: Needs review » Postponed (maintainer needs more info)
msnassar’s picture

I have reproduced this with field collection beta12

james.williams’s picture

Version: 7.x-1.0-beta5 » 7.x-1.x-dev
Assigned: Sokarion » Unassigned
Status: Postponed (maintainer needs more info) » Needs review

Agreed, I hit this with field collection - perhaps specifically because I have a taxonomy autocomplete widget on a term reference field in one of my field collections that appears on the node edit form? (That's where the stack trace leading to the notice takes me.)

james.williams’s picture

Status: Needs review » Needs work

Ah - this looks like its because the ET's widget alter for the taxonomy autocomplete widget gets the handler for the entity type that the widget is on directly, even if that entity type itself isn't configured as enabled for translation. The point of this widget alter hook is (correctly) to translate the term name when the term is configured for translation, regardless of whether the referencing entity is configured for translation or not.

If the referencing entity (in this case, field collection, but could probably be anything) is not configured as enabled for translation, then _entity_translation_process_path_schemes() is never called on the entity info, to normalise the path scheme. So the default elements that would be added to the customised path schemes, from the default path scheme, like the path wildcard, are missing.

So I guess what's wrong here, is that a path wildcard should be in place, but isn't. The above patch's approach of just defaulting it to FALSE isn't optimal, since it would be better to actually have the right path wildcard.

We could put responsibility of ensuring the path wildcards (and other elements that can come from the default path scheme) are set on customised path schemes, onto the modules that set up the customised path scheme. But then why should ET allow any of them to automatically come from the default path scheme? I suspect it would be best for ET to put them there regardless of whether translation is configured as enabled at all? I don't know what the ramifications of that are. Is there anywhere else that cares about the path scheme even when translation is not configured as enabled for an entity/type?

james.williams’s picture

Status: Needs work » Needs review
FileSize
896 bytes

Here's the patch that follows the idea of filling in path scheme defaults even if an entity type is not configured as enabled for translation. It might help us find out what might be affected by this ;-) Hopefully nothing bad!

james.williams’s picture

I have noticed that the ET admin form process the path schemes in order to validate them, regardless of whether the entity type is configured to be enabled for translation. So I suspect this approach is a correct one.

Status: Needs review » Needs work
james.williams’s picture

No idea what's going on with that failure -- I actually get that failure locally, whether I have that patch applied or not :-( Can anyone advise?

james.williams’s picture

Ahah, found the answer - ET tests all passed until Title 7.x-1.0-beta1 was released last week! If I test against the previous stable title module release (alpha9), with or without this patch, the tests succeed :-)

Someone should probably investigate that... I don't know if it's an ET issue or a title module one??

james.williams’s picture

Oh wow, that D8 core patch was not intended here, sorry!

stefanos.petrakis@gmail.com’s picture

Assigned: Unassigned » stefanos.petrakis@gmail.com

Hey James, will have a look at what you report in #13, thanks.
As well as looking into your idea and provided patch for solving this issue.

stefanos.petrakis@gmail.com’s picture

Re-rolling the patch to try and see if that gets rid of the "Patch failed to apply" error from the bot.
@james.williams: The Title-related breaking tests were fixed (for the time).