[PHP 7.2] Incompatible method declarations

I think the first two are easy enough to handle here.

The last one is a bit more tricky but let's try something for that as well.

All green!

+++ b/core/lib/Drupal/Core/Field/FieldItemList.php
@@ -96,18 +96,6 @@ public function filterEmptyItems() {
-  public function getValue($include_computed = FALSE) {

To be honest, I would like this to be fixed in the other way around, i.e. add this parameter to the interface.

We have the need for this parameter in #2824097: Deep serialization for content entities to make an exact snapshot of an entity object's structure based on its current state, which we will need to fix if we want to properly move inline entity form or something similar into core.

I can see that it's a bit vaguely defined/documented at the moment, but since the concept of being computed is part of typed data itself and not added by the entity/field system, I think it's reasonable enough to keep it around.

Thoughts?

@tstoeckler, I think that depends on which interface you would like to add it, because I'm pretty sure anything else other than FieldItemListInterface will be a hard BC break.

Actually I was thinking of TypedDataInterface. Not sure why it would be a hard break, but attaching a patch to try it out. It is somewhat of a BC break, but your patch is as much a BC break as $items->getValue() will have a different return value before and after the patch. So I think there really is no way to do this 100%-BC. We currently have different (default) behavior between equally-named methods on classes implementing the same interface and PHP 7.2 now forces us to make the behavior consistent. We basically just have to pick sides, which one we now declare official.

One a completely unrelated note, I also found DateTimeComputed::getValue($langcode = NULL). Does the green patch above mean that that is never run in any of our tests?

Ahem, this is what I wanted to test.

@tstoeckler, so the BC break I was talking about in #8 is that you can not add an argument to a method, even if it has a default value, without forcing all the subclasses to also add that argument: https://3v4l.org/ijjsg

Ahh you're right. I thought that worked. Hmm... that's annoying.

But I guess you're right there's no way around it I guess. I do think we need to let people know about the different behavior of field items and field item lists, though, so tagging "Needs change record". I had it wrong in #9 I think. $items->getValue() will still return the same thing with the patch, but $items->getValue(TRUE) will not. Maybe we need to provide a dedicated getValueWithoutComputed() method for field items and field item lists? Just thinking out loud here...

In any case, this is rightly needs work not because of my patch but because of DateTimeComputed and I also don't see any FieldItemBase hunk in the patch and that will need to be updated, as well, no?

Maybe we need to provide a dedicated getValueWithoutComputed() method for field items and field item lists?

We already have \Drupal\Core\TypedData\ComplexDataInterface::getProperties($include_computed = FALSE) and I just tested locally with $node->field_tags[0]->getProperties(TRUE); and it returns what you would expect, an array of data type objects (e.g. Drupal\Core\TypedData\Plugin\DataType\IntegerData for 'target_id' and Drupal\Core\Entity\Plugin\DataType\EntityReference for 'entity'), on which you can call getValue() and you get the target_id value and an entity object from the 'entity' property.

I agree that it's a bit complicated to do it like that, but it's certainly possible with existing code in HEAD.

One a completely unrelated note, I also found DateTimeComputed::getValue($langcode = NULL). Does the green patch above mean that that is never run in any of our tests?

Nope, it means that there is no class in core that extends DateTimeComputed with a different argument list for getValue(). So I think this can be tackled in a followup.

I also don't see any FieldItemBase hunk in the patch and that will need to be updated, as well, no?

That's because we don't override getValue() in FieldItemBase so it uses the parent implementation (\Drupal\Core\TypedData\Plugin\DataType\Map::getValue()) ;)

Hmm... I personally would think the DateTimeComputed thing is in scope. Won't this be a problem otherwise if people running PHP 7.2 have a contrib that extends that class? In fact https://www.drupal.org/project/partial_date may or may not do that in D8, I don't remember off the top of my head. On the other hand, I often have a different stance on scope that the committers, so I'll let them make the call on this one. Is not a real big issue either way...

Re FieldItemBase, sorry looked at the wrong PhpStorm window... in our project we have some version of the patch in #2824097: Deep serialization for content entities to make an exact snapshot of an entity object's structure based on its current state applied. So the whole $include_computed stuff is really just dead code as of now?

So the whole $include_computed stuff is really just dead code as of now?

I think so, yes :)

Also, I have no problem with a proactive approach here and change DateTimeComputed in order to save contrib/custom code some trouble.

I just want to mention that according to our BC policy we're not allowed to remove any function parameters:

A backward-compatibility break is a type of API change that requires changes in code implementing a public API. This can be anything from changing the parameters of a procedural function to renaming an interface or removing a hook.

@hchonov, in this case the public API is \Drupal\Core\TypedData\TypedDataInterface::getValue() which doesn't have any parameter.

Also, the call to $item->getValue($include_computed) doesn't do anything because FieldItemBase uses the getValue() implementation of the parent (\Drupal\Core\TypedData\Plugin\DataType\Map::getValue()), which excludes computed properties by default.

So calling $entity->some_field->getValue(TRUE) does *not* return the value of the computed properties in HEAD. Like @tstoeckler said, it's dead code and even a buggy one.

Well but having a public method defined with a parameter on a base class makes it somehow part of our public API, doesn't it?

The issue @tstoeckler referenced shows that the code is not that dead, it is probably not used in core, but it might be used in custom or contrib by creating a custom field item and defining the field item getValue() method with the $include_computed parameter.

I think we could solve the other issue by introducing a dedicated method for the problem described there, but still I don't think that it is a good idea to remove the parameter from getValue().

@hchonov in general I agree that we should not "just" remove the parameter "for fun", but how do you propose do resolve the problem of PHP 7.2 (in)compatibility?

@tstoeckler, what about we do what you've proposed yourself in #6:

To be honest, I would like this to be fixed in the other way around, i.e. add this parameter to the interface.

One way or another we'll have probably a BC break in order to be compatible with PHP 7.2, but at least let's not remove functionality.

See #10 / #12 about that. This will be a hard BC break in that any class that overrides getValue() will now fatal if it does not specify the parameter itself.

The latest patch in #16 is only a BC-break for a theoretical field item list (or field item) (which does not exist in core) that specifies a $include_computed parameter and somehow has logic that depends on that. In that case (and only in that case) the return value of getValue(TRUE) will change with this patch. But that is a *much* smaller break than fataling for anyone who currently overrides getValue() (and doesn't speficy a currently non-existing boolean parameter).

You've modified \Drupal\Core\TypedData\TypedDataInterface::getValue and sure this is too big change and doesn't work :).

But we could redeclare the method in FieldItemListInterface with an optional parameter. This is allowed as we respect the parent interface \Drupal\Core\TypedData\TypedDataInterface::getValue and allow invocation without providing a parameter. This is allowed and PHP is not complaining. Here is an example for this - https://3v4l.org/or3Pj

If anyone is overriding FieldItemList::getValue($include_computed = FALSE) and doesn't define the optional parameter then it is not our fault as we've always had the parameter defined on the method implementation in the base class and our policy is that when we have a base class for an interface, that the base class has to be used instead of implementing the interface - so redeclaring the method in FieldItemListInterface shouldn't be a BC break at all.

Beside that the problem described in the issue summary is in ComputedItemListTrait::getValue, that it doesn't have the $include_computed parameter, not because of a mismatch with the interface:

PHP Fatal error: Declaration of Drupal\Core\TypedData\ComputedItemListTrait::getValue() must be compatible with Drupal\Core\Field\FieldItemList::getValue($include_computed = false) in /home/travis/drupal8/core/modules/system/tests/modules/entity_test/src/Plugin/Field/ComputedTestFieldItemList.php on line 11

Wow, I am learning a lot about PHP in this issue. ;-) I guess that is what @amateescu was talking about in #8, but I didn't get it at the time.

I'm not a release manager, but as far as I can tell, the minor break in #24 is allowed due to the 1-to-1 relationship of FieldItemInterface and FieldItemBase. So from that perspective the patch is good to go.

What annoys me a little bit about the patch is that the $include_computed parameter is on the field item list level instead of the field item level, where it would make more sense semantically. We cannot actually add it in a FieldItemInterface::getValue() because of the BC-problem mentioned above, so I do realize that the patch is the best we can do, but it's still not optimal.

So I think either patch #16 or #24 are fine. Both involve some theoretical breakage for contrib in some edge-cases (#16 breaks contribs that actually pick up the $include_computed in custom field types while #24 breaks contribs that include something similar to ComputedItemListTrait ), but for obvious reasons we need some fix for this. I would personally lean towards #24 simply because it retains a little bit more flexibility in getValue() that we might be able to use in other issues, but it's absolutely not a show-stopper if we were to drop it (i.e. if we went with #16).

Would love to hear what @amateescu thinks.

#16 breaks contribs that actually pick up the $include_computed in custom field types

That's not true. #16 does not break anything in contrib/custom code for the reason that I already explained in #19.

Current HEAD just passes the $include_computed parameter to FieldItemBase::getValue() which does not respect or use it, so if contrib actively relies on that parameter that code is either 1) broken, because the parameter is not used by core or 2) they also provide a custom field item class which uses the parameter, in which case their code will continue to work just fine with the patch from #16.

On the other hand, #24 is as much of a BC break as #10, just on a different level.

Sorry, but I have to disagree with #26.

#16 breaks contribs that actually pick up the $include_computed in custom field types

That's not true. #16 does not break anything in contrib/custom code for the reason that I already explained in #19.

Current HEAD just passes the $include_computed parameter to FieldItemBase::getValue() which does not respect or use it, so if contrib actively relies on that parameter that code is either 1) broken, because the parameter is not used by core or 2) they also provide a custom field item class which uses the parameter, in which case their code will continue to work just fine with the patch from #16.

It is true. I am talking about your case 2), but the code will not continue to work. Indulge me for a second:

// I have a module that provides a custom field type with computed
// properties.
// Somewhere in my module I have a field item list of my custom type
// and I want to get its value:
$value = $items->getValue()
// PhpStorm, now tells me there is a $include_computed parameter.
// Thinking about it, I realize I *do* want the computed properties,
// so I change it to:
$value= $items->getValue(TRUE);
// Now I'm disappointed that the computed properties are not contained
// so I look into FieldItemList::getValue() and see that it calls down to my
// field item class, so I think that maybe I'm doing something wrong.
// I end up putting the following in my field item class:
public function getValue($include_computed) {
  $value = parent::getValue();
  if ($include_computed) {
    $value['foo'} = $this->foo;
  }
  return $value;
}
// Even though I could have expected core to do this for me, this was easy
// enough and I now go on with my life and everything works as expected.

This is exactly the case that breaks. I'm not saying it's not far-fetched, and I'm also not saying it's not an acceptable risk, but it's simply not correct to say that #16 doesn't cause breakage.

On the other hand, #24 is as much of a BC break as #10, just on a different level.

Again, I have to disagree with this. Our backwards-compatibility policy explicitly allows changes of interfaces where there is a base class that can be assumed to implemented, which is the case here. So there are two possible breaks:

1. Some specialized list class (or trait that is used in a specialized list class) overrides getValue() without a $include_computed parameter
2. Some specialized list class (or trait that is used in a specialized list class) does not extend FieldItemList and does not add an $include_computed parameter

This is a much smaller risk than the breakage caused by #10. It's fair if you want to argue that it's still a bigger risk than #16, but saying it's just as risky as #19 is simply false.

Here's a third approach. It keeps the $include_computed parameter in FieldItemList and adds it to ComputedItemListTrait. I think it is the approach that sufficiently solves the scope of this issue but causes the least disruption. We can then discuss what to do with the currently dead code in FieldItemList::getValue() in a separate issue.

That said, even having written #27 I'm not that strongly against #16, I just felt I needed to clear up my point of view a bit.

It is true. I am talking about your case 2), but the code will not continue to work.

You're right, in #26 I was assuming that the field type also has a customized field item list class with an overridden getValue() method. Not sure why I had that in mind, but anyway, that's the reasoning for my comment.

If we really really want to not break the far-fetched scenario from #27, we could keep the custom getValue() implementation from FieldItemList and use func_get_args() to detect if we received an argument and pass it along like we do in HEAD.

However, looking at your example a bit, how would PHPStorm tell you about the $include_computed parameter since \Drupal\Core\Entity\FieldableEntityInterface::get() says the return value is FieldItemListInterface, which does not have the parameter? :)

This is a much smaller risk than the breakage caused by #10. It's fair if you want to argue that it's still a bigger risk than #16, but saying it's just as risky as #19 is simply false.

I didn't say that it's just as risky as #19, I just said the BC break is moved to a different level.

+++ b/core/lib/Drupal/Core/TypedData/ComputedItemListTrait.php
@@ -39,9 +39,9 @@ protected function ensureComputedValue() {
-  public function getValue() {
+  public function getValue($include_computed = FALSE) {

The problem with this (and also with #24) is that the trait sits at the item list level (typed data level), not at the field item list level (entity field API level), a thing that I tried very hard to achieve in the patch that introduced it, and this change breaks that. We would need to write a new trait which extends the existing one if we want to add that parameter.

The problem with this (and also with #24) is that the trait sits at the item list level (typed data level), not at the field item list level (entity field API level), a thing that I tried very hard to achieve in the patch that introduced it, and this change breaks that.

Ahh, I totally did not realize that. It's funny, I actually tripped on the missing "Field" in the name of that trait, but thought that was just omitted for brevity. That's really cool and actually makes a lot of sense, nice work! Or rather: Even nicer work than I realized! ;-)

We would need to write a new trait which extends the existing one if we want to add that parameter.

Actually we wouldn't need to write such a trait. The patch in #24 actually works fine, even if the trait is used outside of the field context (see https://3v4l.org/1YgNV). I can see, however, the argument that it makes the patch a lot more ugly than I previously thought as it forces us to introduce something from the field item list level to the generic typed data level. Same for #28.

So, all having been said, I guess you still consider #16 the best solution? If so, feel free to re-upload it and I will RTBC. I had discussed this with @hchonov in person yesterday and he confirmed that he would not veto #16 going in assuming that is what you prefer.

EDIT: added 3v4l.org proof I already had in my clipboard... ;-)

We would need to write a new trait which extends the existing one if we want to add that parameter.

I guess what @amateescu means is that we need two different traits for the two different contexts - field items lists and typed data item lists. I think this would be the best way where we have a consistency - and allows us to further extend the behaviour of field item lists without having to adjust TypedDataInteface.

Like @tstoeckler has said I'll not prevent #16 from getting in. but I would prefer #24 or #28 with adding a second trait for field lists and I think this is fine, because they use different namespaces.

The only thing that bothers me about #16 is that in our system we have custom code using the $include_computed parameter and relies on the parent passing it to the children. We'll be forced to rewrite the part in our code that relies on the parameter, but I'll be surprised if we are the only ones which will have to do this.
Additionally such a change is impossible for 8.4.x, because we have to provide a time window for contrib and custom to adjust to the BC break. I am not sure if it is acceptable to solve this only for 8.5.x as PHP 7.2 is in RC6 currently, which means it will be released before Drupal 8.5.0.

Sure, the solution with adding a new trait at the field item list level will work fine, my problem with that approach is that we would introduce it just for this method and just to keep an argument that doesn't even work in core.

Like I mentioned in #14, there is a way to get all the values from a field item list, including the computed ones, which works fine and is actually supported by core, so I really don't understand all the fuss with keeping this broken method/parameter...

For that reason, I'm re-uploading the patch from #16 as suggested in #30.

there is a way to get all the values from a field item list, including the computed ones, which works fine and is actually supported by core, so I really don't understand all the fuss with keeping this broken method/parameter...

The problem is that it is a behaviour change, which forces a developer to update custom code and yes we'll have to do this for our project.

We could always change things in Drupal and break BC while offering another way of solving problems which involves a developer work, but I am not sure if this is an acceptable when we offer long term support. This is a change that has to be signed by a release manager.

Like I've said, I'll agree on removing the parameter.

But there remains the question - are we allowed to break BC in 8.4.x and give no time window to developers to adapt or we disallow using PHP >= 7.2.x with < Drupal 8.5.0?

@catch, @effulgentsia, @larowlan, @xjm and I discussed this issue today. We agreed it should be considered a critical as it will break our test suite, as soon 7.2 becomes the default testing environment.

Merging in the last patch from #2927173: Fatal error on PHP 7.2: Method signature compatibility strictly enforced, since it's now covered by this issue.

Cross-talk with #2870465: Convert web tests to browser tests for user module on the user web tests; needs reroll.

No good way to do an interdiff on a re-roll, but it looks like the fixes to the user views test were included in the conversion, and can be removed here completely.

+++ b/core/modules/views/src/Tests/FieldApiDataTest.php
@@ -35,7 +35,7 @@ class FieldApiDataTest extends FieldTestBase {
     parent::setUp(FALSE);

@cburschka you didn't include my fix from the other issue. This is going to result in a failed test. #35 merged in your last patch from the other issue - not the actual last patch.

Oh thanks - forgot to put the last patch in my local branch before merging.

Seems to need a reroll after #2916300: Use ComputedFieldItemListTrait for the path field type. It looks like this is just another removal - PathFieldItemList no longer has ::getValue(), so we no longer need to remove the $include_computed argument from it (and the replacement method ComputedItemListTrait::getValue() doesn't have it in the first place).

As a side note, I found the relevant PHP issue: https://bugs.php.net/bug.php?id=73987, which indicates that it was meant to be enforced all along, and the fact that we got away with it was a bug:

When PHP validates method signatures for compatibility, if a method is defined in an interface then compatibility is measured against the interface and any parent method's signature is ignored. This leads to inconsistencies...

1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -250,7 +250,7 @@ public function id() {
   -  public function preSave(EntityStorageInterface $storage, $update = TRUE) {
   +  public function preSave(EntityStorageInterface $storage) {
   

   This looks like a c&p error from a postSave() which has the $update param. It was introduced in #2144919: Allow widgets and formatters for base fields to be configured in Field UI

2. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -95,18 +95,6 @@ public function filterEmptyItems() {
   -  /**
   -   * {@inheritdoc}
   -   * @todo Revisit the need when all entity types are converted to NG entities.
   -   */
   -  public function getValue($include_computed = FALSE) {
   

   As discussed at length by @amateescu, @hchonov and @tstoeckler this is the most controversial part of the changes. This removal is fine for core because this method apart from that incorrect param is an exact copy of \Drupal\Core\TypedData\Plugin\DataType\ItemList::getValue which FieldItemList extends from. However reading @hchonov's comments makes me think that at least we should have a change record to tell people what do.

3. +++ b/core/modules/datetime/src/DateTimeComputed.php
   @@ -37,7 +37,7 @@ public function __construct(DataDefinitionInterface $definition, $name = NULL, T
   -  public function getValue($langcode = NULL) {
   +  public function getValue() {
   

   Similar changes made in #2073217: Remove the $langcode parameter from the entity view/render system, #1869562: Avoid instantiating EntityNG field value objects by default

   This was added in #2083785: Add support for determining which field properties are cacheable - no idea why since the interface didn't have the param but as #2073217: Remove the $langcode parameter from the entity view/render system shows perhaps it was a c&p error. Fix looks good.

4. +++ b/core/modules/layout_builder/src/Form/UpdateBlockForm.php
   @@ -39,7 +39,7 @@ public function getFormId() {
   -  public function buildForm(array $form, FormStateInterface $form_state, EntityInterface $entity = NULL, $delta = NULL, $region = NULL, $uuid = NULL) {
   +  public function buildForm(array $form, FormStateInterface $form_state, EntityInterface $entity = NULL, $delta = NULL, $region = NULL, $uuid = NULL, array $configuration = []) {
   
   +++ b/core/modules/migrate/src/Plugin/migrate/source/EmptySource.php
   @@ -58,7 +58,7 @@ public function getIds() {
   -  public function count() {
   +  public function count($refresh = FALSE) {
   
   +++ b/core/modules/system/tests/modules/menu_test/src/Plugin/Menu/LocalTask/TestTasksSettingsSub1.php
   @@ -12,7 +13,7 @@ class TestTasksSettingsSub1 extends LocalTaskDefault {
   -  public function getTitle() {
   +  public function getTitle(Request $request = NULL) {
   
   +++ b/core/modules/tracker/src/Tests/Views/TrackerTestBase.php
   @@ -41,8 +41,8 @@
   -  protected function setUp() {
   -    parent::setUp();
   +  protected function setUp($import_test_views = TRUE) {
   +    parent::setUp($import_test_views);
   

   The getTitle(), setUp(), count(), buildForm() changes are simply where the param is not used. In previous PHPs this could be omitted in PHP7.2 it can't be.

Here is a version of the patch from #42 for 8.4.x.

@pfrenssen see #31 and #33 where I've expressed my concerns about making this change in 8.4.x. It is too big change to allow it in 8.4.x.

I am wondering if we should directly introduce here the replacement for ::getValue($include_computed)? I mean we cannot simply write in the change record, that we've removed the method and do not offer any core functionality for retrieving the computed properties of all items.

@hchonov, we already have a proper (i.e. working as documented) way for retrieving the computed properties of all items in core, see #14.

@amateescu, like you've mentioned it there:

I agree that it's a bit complicated to do it like that, but it's certainly possible with existing code in HEAD.

I just think it would be nice to provide a way of doing it by just calling one method on the item lists. I think it will be much easier for a lot developers just to replace the method call wherever needed.

#14 shows also only how you retrieve all properties from a single item, not from an item list, which means if we don't introduce a method for doing this job then every single project making use of this functionally that we are removing will have to reimplement the same logic on its own.

@pfrenssen see #31 and #33 where I've expressed my concerns about making this change in 8.4.x. It is too big change to allow it in 8.4.x.

OK but then we explicitly should declare in composer.json that Drupal 8.4.x is not compatible with PHP 7.2. Currently it says that it is compatible with PHP >=5.5.9.

We still have to declare in composer.json that Drupal 8.4.x isn't compatible with PHP 7.2 as mentioned by @pfrenssen in #49. But we need a green light from a release manager about this.

These are the only run-time changes that are necessary to have Drupal 8 compatible with 7.2. All the other changes for PHP 7.2 are test only.

Re-uploading #42 so the correct patch to review is last on the issue.

I think we need a CR to document the change and tell people how to get include computed fields in a core-supported fashion. With respect to the BC break there has always been a proviso that we can decide to break BC for critical changes such as this which I think makes sense since in core this functionality does not even work.

Re #53 and earlier: Since the same patch won't work for both 8.4.x and 8.5.x, we should make 8.5.x work, then say 'needs backport' for 8.4.x.

Then we can make an 8.4.x patch and apply it.

The patch does apply for 8.5.x. The PHP 7.2 test is still queued so I can't say it's passing...

1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -250,7 +250,7 @@ public function id() {
   -  public function preSave(EntityStorageInterface $storage, $update = TRUE) {
   +  public function preSave(EntityStorageInterface $storage) {
   

   EntityInterface::preSave() confirms this. Dunno what to add, other than adding an update flag seems to violate the API from the interface, so it should go, in principle. If that's both needed and not BC compatible then update the interface for Drupal 8.6.

2. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -95,18 +95,6 @@ public function filterEmptyItems() {
   -   * {@inheritdoc}
   -   * @todo Revisit the need when all entity types are converted to NG entities.
   -   */
   -  public function getValue($include_computed = FALSE) {
   

   'NG entities'... As in Drupal 8.0.0 entities?

3. +++ b/core/modules/field/src/Tests/Views/HandlerFieldFieldTest.php
   @@ -41,8 +41,8 @@ class HandlerFieldFieldTest extends FieldTestBase {
   -  protected function setUp() {
   -    parent::setUp();
   +  protected function setUp($import_test_views = TRUE) {
   +    parent::setUp($import_test_views);
   

   There are a bunch like this.

   Why is this allowed if BTB::setUp() doesn't take arguments? Is it because BTB is abstract?

Other than that, yes, still needs a CR, maybe more than one. Some of these seem like subtle architectural changes and should maybe be their own issues, such as the conversation about getValue() above.

Re #56.3 - You can add arguments - you just can't add them and them take them away again if you inherit from something that has already added them.

Re the CRs - there is no architectural change other than the include_computed and even that is not a real architectural change because at the moment if you call that method with different arguments in core it does absolutely nothing. This is PHP 7.2 enforcing PHP class inheritance on things that it has always considered a bug. I really don't think CRs are needed here or offer any benefit. If you test your software on PHP7.2 you'll get a nice clear error message.

There was a conflict with #2926914: Rewrite \Drupal\layout_builder\Section to represent the entire section, not just the block info so I've re-rolled the patch.

Added PHP 7.2 testbot.

@Mile23 adding PHP7.2 testbot on this issue doesn't prove too much until #2927806: Use PHPUnit 6 for testing when PHP version >= 7.2 lands.

I think we should add a change record noting that the $include_computed flag has been removed, highlighting that it didn't really work anyway, and pointing to the correct way to do it, as seen in comment #14.

Also, I think that means we can't backport this to 8.4.

I've created an initial draft CR for the removed parameter from FieldItemList::getValue(): https://www.drupal.org/node/2932554 .

And yes we can't make this change in 8.4.x. as e.g. we have systems running on 8.4.x and using this functionality.

Okay I think we're done here. We've got consensus from the entity system maintainers and all the other changes are minor and not controversial at all.

I've added #2932574: Indicate Drupal 8.4.x is not compatible with PHP7.2 to discuss marking Drupal 8.4.x as incompatible.

Hopefully this will have a green result on PHP 7.2!

Lost a comment. I've responded to the follow-up. I think it's safer to leave this on 8.5.x and the 8.5.0 alpha is in mid-January. It's a slightly longer window than I'd prefer but could be a lot worse. Committed/pushed to 8.5.x, thanks!

After updating to latest version in 8.4.3, I am having this issue when edting a node.
Do you recommend to apply the patch as mentioned in #57?

Regards.

@beautifulmind are you on PHP 7.2 - Drupal 8.4.x doesn't work on PHP 7.2. There are several other patches that need to be applied for it work. The best thing is to not move to PHP 7.2 until 8.5.0 is out.

@AlexPott
Thank you for the confirmation. I had this doubt when I encoutered the error.

Regards.

For anyone finding this thread and scrolling to the bottom: #2932574: Indicate Drupal 8.4.x is not compatible with PHP7.2

publish the change record