Fix ContentEntityBase::__get() to not return by reference

Documentation is not the answer here.

Updated issue summary.

You arguing about the complexity of the BC-mode. Yes BC-mode is unnecessary complex as it translates from an old to a new API. Yes, that's very hard to follow and understand, but it's not supposed to stay.

berdir suggested to add the new variant here, so I do:

$entities[$row->entity_id]->getTranslation($row->langcode)->{$field_name}[] = $item;

or just

$entities[$row->entity_id]->{$field_name}[] = $item;

if the langcode is default language anyway.

Isn't this just "remove the backwards compatibility layer" if so that's #1818580: [Meta] Convert all entity types to the new Entity Field API.

>NG system relying on &__get

The NG system does not rely on that?

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

Back to needs more info. There's still nothing specific here that's not due to the backwards compatibility layer.

Updated issue summary.

Tagging to get this to show up in all the relevant lists.

Sure. Now that we don't need it for BC support, let's resolve that @todo.

No, that's not possible as long as we have code the one mentioned in the @todo, I left that there for a reason :)

Well, let's at least see how far the rabbit hole goes.

Sure :)

The translation part is handled in #1916790: Convert translation metadata into regular entity fields, no idea what to do with ->content.

Expanding the scope of this patch to track down other cases of non-field properties.

Grrrr. I give up. Anyone have an idea how to reproduce bot's installation failure?

24: entity_get_not_by_ref-1977266-24.patch queued for re-testing.

I wasn't able to reproduce the install fail but here is a reroll for a conflict with #2028037: Expand FeedInterface with methods.

Missed this added property, going to cancel the last one.

Rerolled. No interdiff possible.

#18:

Expanding the scope of this patch to track down other cases of non-field properties.

That's quite a different beast and the patch did not install successfully since then. Can we go back to the patch in #17 for this issue?

The ability to support non-fields is what separates __get() from get(), if we don't want that, we can simply call get() from there. But IMHO, that's a different issue.

I'd rather have support for non-fields than weird public properties like the recent pages have.

Well I can install locally, so I don't know where to go from here.

In this last patch, I tried to document where the non-fields were called from.

In the ConfigEntity world, we removed as many of these as possible, and use get()/set() explicitly with protected properties...

So can I, but wouldn't be the first time bot somehow works differently. Although we can try again now that 8.x is running on 5.4.

But I'm worried that doing it all at once is going to delay this patch a lot. And as mentioned, then we just need to pass to $this->get() in __get().

34: content-entity-1977266-34.patch queued for re-testing.

Nevermind then.

Actually, even going back to #17 doesn't help much. Because that only passes with having public properties that are used elsewhere, some in optional modules.

What we need is actual solutions for the values that were identified there. For translatoin, we have #1916790: Convert translation metadata into regular entity fields.

I'm not sure what to do about the others?

So, with regular PHP objects, you can do $foo->some_undeclared_array[] = 'additional value'. The question is, do we want to allow the same for entities, so that entities behave just like all other PHP objects, or do we want to prevent that for entities, since it's bad OOP practice in general for outside code to add arbitrary properties to objects, and potentially especially bad to do that on entities?

If we want to treat entities as any other PHP object and allow it, then __get() *should* return by reference, but the issue summary says that that's confusing, but I'm not entirely clear why.

But, if we want to make the entity data model strict, to encourage/force good OOP practice at least with regards to entities, then we need to find every occurrence in HEAD that currently violates that and fix it. In some cases, that means turning something into a field. In other cases, it might mean storing temporary data (e.g., $entity->content) somewhere other than on $entity.

But, if we want to make the entity data model strict, to encourage/force good OOP practice at least with regards to entities, then we need to find every occurrence in HEAD that currently violates that and fix it. In some cases, that means turning something into a field. In other cases, it might mean storing temporary data (e.g., $entity->content) somewhere other than on $entity.

I am all for this second approach, but honestly I don't know whether it's still viable. Anyway, it's THE thing to do IMO, and what I've been aiming for since we started working on D8 entities.

I agree that we should encourage good OOP practice and should not support writing internal field values, as the issue summary example shows.

However, supporting some temporary non-field data on the entity is ok imho - it's just what you would expect to work from a random object. I'd not say it's best practice to do it, but still it makes sense to me to let the object behave as normal as possible.

I'd rather have support for non-fields than weird public properties like the recent pages have.

Agreed.

But problem is, that reference allows changing $this->field['foo'] also unless we add some useless intermediate "copy"? Or should we go with the middle-way would be to support non-fields but not by reference? That means you cannot do $entity->content[] = .. though, but at least writing temporary variables works.

I do not think this issue is critical though, so lowering to major.

Agree with not critical, thanks.

->content is (almost?) gone now with the entity build/view cache refactoring, but there are still a few things left that need the by-reference stuff.

This could really use an issue summary update.

Discussed with @catch, @xjm, @effulgentsia, @amateescu, @Berdir, @plach, @tstoeckler. We agree that this is not a major bug because the issue summary doesn't really describe a bug. Yes the BC layer added for entities is complex and it would be great to remove it and some of the complexity and magic but at best this is a task. This issue should be re-scoped to deprecate it and remove core usage. Hopefully we can remove some of this in Drupal 9.

This looks like a dup of the other nowadays.