There are several places in core that use this pattern to implement lazy loading (this snippet from core/lib/Drupal/Core/Batch/BatchStorage.php):
public function create(array $batch) {
// Ensure that a session is started before using the CSRF token generator.
$this->session->start();
$try_again = FALSE;
try {
// The batch table might not yet exist.
$this->doCreate($batch);
}
catch (\Exception $e) {
// If there was an exception, try to create the table.
if (!$try_again = $this->ensureTableExists()) {
// If the exception happened for other reason than the missing table,
// propagate the exception.
throw $e;
}
}
// Now that the table has been created, try again if necessary.
if ($try_again) {
$this->doCreate($batch);
}
}
I think that is a little more complicated than necessary, and I think it is worth cleaning this up, since exception handling should be as transparent as possible. I would like to replace the above code with this, eliminating the $try_again variable:
public function create(array $batch) {
// Ensure that a session is started before using the CSRF token generator.
$this->session->start();
try {
// The batch table might not yet exist.
$this->doCreate($batch);
}
catch (\Exception $e) {
// If there was an exception, then try to create the table.
if ($this->ensureTableExists()) {
// Now that the table has been created, try again.
$this->doCreate($batch);
}
else {
// If the exception happened for other reason than the missing table,
// then propagate the exception.
throw $e;
}
}
}
I first noticed this pattern when reviewing #2664322: key_value table is only used by a core service but it depends on system install, but @dawehner pointed out this this pattern is already used elsewhere in core. In that issue, the pattern is slightly different: the foo() method does the work itself, and calls itself from the catch block, instead of having a separate method doFoo() that does the actual work.
I believe that the code is functionally identical, so this should not make any API changes.
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | interdiff_26-27.txt | 522 bytes | vsujeetkumar |
| #27 | 2721271-27.patch | 9.12 KB | vsujeetkumar |
| #26 | interdiff_25-26.txt | 2.16 KB | vsujeetkumar |
| #26 | 2721271-26.patch | 9 KB | vsujeetkumar |
| #25 | interdiff_12-25.txt | 5.86 KB | vsujeetkumar |
Comments
Comment #2
benjifisherA related suggestion: maybe it deserves a separate issue? The lazy-loading pattern uses the
ensureTableExists ()method, so maybe that should be moved to a separate trait for consistency. This pattern also uses thecatchException()method, which could also be added to that trait. If we are already using these two methods in five different places, maybe it is time to derive them from a common source.Comment #9
dawehnerI'm having a look at that.
Comment #10
dawehnerHere is a patch which adds a trait and uses it in two places.
Comment #11
johnwebdev commentedwhitespace
missing visibility
the indentation seems off
Can we really delete that method? Shouldn't it call ensureTableExists?
Should we have a follow- up for all other occurrences, or fix them in this issue?
Comment #12
johnwebdev commentedShould this be abstract? I think $this->connection is the property holding the connection is called often though. But I'm not sure whetever a trait should assume properties exists?
Comment #13
dawehner@johndevman Thank you for your super quick review! I hope I addressed all the points.
Comment #14
benjifisherIt looks as though the patch in #12 includes some unwanted parts. For example, it removes
core/lib/Drupal/Component/Plugin/ConfigurableTrait.php, so I am surprised that the testbot does not simply report that the patch fails to apply. (Does it try to reverse the patch in this situation?) My guess is that you diff'ed two different feature branches instead of your current branch and core.Note that the patch in #10 is 6.55 KB, but the one in #13 is 33.07 KB.
Comment #15
johnwebdev commentedNot sure what happened in the last patch, but I found some more nits.
Two spaces
Misses the summary.
Needs docblock.
Misspelled lazily.
Check(s) and remove /semaphore/
Should probably have a proper docblock as well.
Comment #24
bhanu951 commentedComment #25
vsujeetkumar commentedRe-roll created, Please have a look
Comment #26
vsujeetkumar commentedFixed CCF issue, Addressed #15, Please have a look.
Comment #27
vsujeetkumar commentedFixed the CCF issue.