datetime_range throwing error in datetime_range_entity_view_display_presave()

Thanks for reporting

Issue should be against 11.x as the current development branch

Also as a bug will need test coverage.

That said may need to do some backtracing to see why that value is empty vs just an isset check. To make sure we aren't masking a larger issue.

Thanks for the feedback.

Issue should be against 11.x as the current development branch

As written in the summary - For 11.x this is obsolete due: 3442162 📌 Remove redudundat hook_ENTITY_TYPE_presave() and ViewsConfigUpdater methods Active
There's no longer any similar code - at least not that I could find any.

Also as a bug will need test coverage.

Does it? Because the current behavior ignores/violates the contract set forth by the method called.
As outlined in the summary:
According to \Drupal\Component\Plugin\Discovery\DiscoveryInterface::getDefinition() it is valid to return NULL :

   * @return mixed
   *   A plugin definition, or NULL if the plugin ID is invalid and
   *   $exception_on_invalid is FALSE.
   *
   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
   *   Thrown if $plugin_id is invalid and $exception_on_invalid is TRUE.
   */
  public function getDefinition($plugin_id, $exception_on_invalid = TRUE);

I'm not sure if it really is necessary to have a test case for using the API not entirely correct.

That said may need to do some backtracing to see why that value is empty vs just an isset check.

I partially agree, at least on behalf of extra_field_plus → / Frontend Editing → but not from the perspective of the datetime_range module.
The value gotten is NULL, which is a valid value for our purpose. If that wasn't meant to be NULL that's an issue for extra_field_plus → / Frontend Editing → .
Or one could argue that there should be somewhere a warning triggered by some core component about that - but this isn't the scope of this issue. Happy to create a follow up issue if required.

Will leave in review but still believe needs a test case.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Will leave in review but still believe needs a test case.

Thanks for the feedback. I'll look into that. I suppose a simple UnitTest should do.

Added unit test. Test only branch would fail. I guess testing pipeline will only work with MR's but I don't think I want to clutter MRs with test only. What's the best practice nowadays for test only changes to demonstrate bugfix & test coverage?

Btw. that unit test setup doesn't seem cool.
Happy to take pointers to enhance it...

I'm also encountering this on a project (current latest 10 release). Diff did not apply to that tagged release so I could not test yet. If I find time I'll see if I can make a patch for 10.x

Thanks for the MR, I left my feedback, please check. Set back to NW.

Changing version to latest 10.x as I see in the IS that this is obsolete for 11.x