- 🇺🇸United States smustgrave
Is this something that will require a test case?
- 🇦🇺Australia mstrelan
I'm not sure if there's a performance cost, but even if there isn't, it's clearer if the code just gets the one theme it wants.
There is no performance cost, because
ThemeHandler::getTheme
does the exact same thing. But I agree it would be clearer.The latest patch is catching an
InvalidArgumentException
but that changed to\Drupal\Core\Extension\Exception\UnknownExtensionException
in #2940203: Use dedicated Exception classes for extension system → . FWIW I'm not sure we should really be catching the exception, or if we are we should do something more useful than just returning an empty array. Otherwise you never really find out that something is broken.Needs to be converted to an MR against 11.x.
Is this something that will require a test case?
Wouldn't have thought so, it should already be tested somewhere, we're just changing the implementation details.
- First commit to issue fork.
- Merge request !12588system_region_list() could use getTheme() instead of listInfo(). → (Open) created by mrinalini9
- 🇦🇺Australia mstrelan
Let's catch \Drupal\Core\Extension\Exception\UnknownExtensionException instead
use \Drupal\Core\Extension\Exception\UnknownExtensionException instead of \InvalidArgumentException.
please review.The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.
- 🇦🇺Australia mstrelan
The issue summary mentions twice that the current approach is inefficient, but that's not the case. As per #22
::getTheme
already calls::listInfo
, so this is only improving clarity.Also, instead of try/catch why don't we use
\Drupal::service('theme_handler')->themeExists()
and return early if that is false. - First commit to issue fork.
- 🇬🇧United Kingdom nexusnovaz
I fear as though i have messed up the pulling of 11.x. Though i did make the change to remove the try catch clause and replacing it with a guard clause. Would someone be so kind as to explain what to do in this situation? For example, i did a
git pull origin 11.x
thengit config pull.rebase false
once it gave me some warning, then the git pull once again. Sorted the conflict (or so i thought) and pushed.