Currently, we have a lot of code that does something like the following:
if ($index->hasValidTracker()) {
return $index->getTrackerInstance()->…();
}
While this will never throw an exception (or only in extreme edge cases), IDEs still complain about an unchecked exception because getTrackerInstance() is documented to throw an exception if the plugin could not be loaded. As a consequence, it is sometimes impossible to write clean code using the Search API without including unnecessary try/catch constructs.
Add variants like getTrackerInstanceIfAvailable() that return NULL instead of throwing an exception and use that where appropriate.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
drunken monkeyAdded an MR and a change record, please review.
Comment #4
borisson_I am personally not a huge fan of having to do
NULL-checks (or null-safe chaining) instead and this increases our api surface by quite a bit.The chance this is introducing bugs is relatively low though, so not opposed to this based on that.
Comment #5
drunken monkeyThanks a lot for weighing in on this, Joris! Always good to have some input when changing the API.
In my mind, the main use case is actually the one from the change record: You’re already checking for, e.g.,
$index->hasValidTracker()but then the IDE still marks your call to$index->getTrackerInstance()as potentially throwing an exception.I agree, just replacing exception handling with needing to explicitly check for
NULLwould be just a huge step back by several decades, so definitely don’t want to encourage that.With the null-safe operator, I personally feel like there is a very handy shortcut for when you just need the entity/plugin for a single call anyways. If you feel that this makes the code harder to read, I could also revert those changes to the code that use the new methods that way, that’s no problem.
If you think the additional methods encourage bad patterns in code then that’s of course a larger issue, we don’t want to do that.
Went slightly insane trying to find the cause of the test failure in
IntegrationTest::testFramework(), but finally found and fixed it: The change toIndexFormuncovered a completely unrelated bug where usingset()to update some of our entity properties would lead to an inconsistent internal state.Should be fixed now, I also improved the test coverage a bit to produce a more useful error in case this would happen again.
So, if we end up not going ahead with this change, we should definitely still fix that bug.
Comment #6
drunken monkeyOK, the fix for this unrelated bug is a bit more involved, so moving that to its own issue, #3549460: Fix hidden bug when entity properties are updated via ConfigEntityBase::set(). (Apparently,
set()is also called during entity save, which causes this new bug and corresponding test failure.)Comment #7
drunken monkeyThere is now a separate MR for that bug in #3549460: Fix hidden bug when entity properties are updated via ConfigEntityBase::set(), I added that as a dependency of this issue’s MR. (Before that one is merged, this one won’t pass anyways.)
Comment #8
borisson_This is a very valid usecase indeed. I agree that will make it easier and since this is such a low impact bug-wise, while still increasing the DX, it's probably worth getting this in.
Comment #9
borisson_Comment #10
drunken monkeyJust merged #3549460: Fix hidden bug when entity properties are updated via ConfigEntityBase::set() and the pipeline is passing now.
So, merged. Thanks again for your input on this, Joris!
Comment #13
drunken monkey