Description
Summary
Hi
As of lately, I've started scanning phtml files with phpstan in our projects, to try to discover potential problems early before they end up in production.
One thing that annoys me is that a lot of type annotations in Magento core are wrong or not accurate enough.
One such example is when calling createBlock on the layout, you pass it the class of the block to create and it returns an instance of this class, but phpstan can't figure out exactly which class the method returns.
We can solve this by using generics in docblocks. You can read more about it here.
Examples
A concrete example can be seen here, in this file: https://github.com/magento/magento2/blob/ebd6774d6952cdd0a4168c50d3211eef988ad711/app/code/Magento/Customer/view/frontend/templates/address/edit.phtml
The $_company variable has type Magento\Customer\Block\Widget\Company, but phpstan thinks its type is Magento\Framework\View\Element\BlockInterface, due to how it's defined in the docblock
You can see this when we invoke phpstan on that phtml file with level 2:
$ ./vendor/bin/phpstan analyse -c dev/tests/static/testsuite/Magento/Test/Php/_files/phpstan/phpstan.neon --level=2 app/code/Magento/Customer/view/frontend/templates/address/edit.phtml
1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
------ --------------------------------------------------------------------------------------------
Line
------ --------------------------------------------------------------------------------------------
46 Call to an undefined method Magento\Framework\View\Element\BlockInterface::isEnabled().
47 Call to an undefined method Magento\Framework\View\Element\BlockInterface::setCompany().
50 Call to an undefined method Magento\Framework\View\Element\BlockInterface::isEnabled().
51 Call to an undefined method Magento\Framework\View\Element\BlockInterface::setTelephone().
54 Call to an undefined method Magento\Framework\View\Element\BlockInterface::isEnabled().
55 Call to an undefined method Magento\Framework\View\Element\BlockInterface::setFax().
------ --------------------------------------------------------------------------------------------
Proposed solution
If we introduce the generics syntax in the docblock on the createBlock method, we can fix this, proposal:
diff --git a/lib/internal/Magento/Framework/View/Layout.php b/lib/internal/Magento/Framework/View/Layout.php
index eeba7485e04..691f0df64c6 100644
--- a/lib/internal/Magento/Framework/View/Layout.php
+++ b/lib/internal/Magento/Framework/View/Layout.php
@@ -768,10 +768,12 @@ class Layout extends \Magento\Framework\Simplexml\Config implements \Magento\Fra
/**
* Block Factory
*
- * @param string $type
+ * @template T of \Magento\Framework\View\Element\AbstractBlock
+ *
+ * @param class-string<T> $type
* @param string $name
* @param array $arguments
- * @return \Magento\Framework\View\Element\AbstractBlock
+ * @return T
*/
public function createBlock($type, $name = '', array $arguments = [])
{
diff --git a/lib/internal/Magento/Framework/View/LayoutInterface.php b/lib/internal/Magento/Framework/View/LayoutInterface.php
index 3a63b5ccc9e..7bfe4d75ff9 100644
--- a/lib/internal/Magento/Framework/View/LayoutInterface.php
+++ b/lib/internal/Magento/Framework/View/LayoutInterface.php
@@ -187,10 +187,12 @@ interface LayoutInterface
/**
* Block Factory
*
- * @param string $type
+ * @template T of Element\BlockInterface
+ *
+ * @param class-string<T> $type
* @param string $name
* @param array $arguments
- * @return Element\BlockInterface
+ * @return T
*/
public function createBlock($type, $name = '', array $arguments = []);After this change, if we execute phpstan, it's able to detect the correct type(s) of blocks and is no longer confused:
$ ./vendor/bin/phpstan analyse -c dev/tests/static/testsuite/Magento/Test/Php/_files/phpstan/phpstan.neon --level=2 app/code/Magento/Customer/view/frontend/templates/address/edit.phtml
1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
[OK] No errors
There are probably other places in Magento core where we can introduce generics in docblocks, it's a matter of discovering where we need to change this and then introduce them in the core.
This would significantly increase the ease of which static analysis tools can run on the codebase without being confused.
Release note
No response
Triage and priority
- Severity: S0 - Affects critical data or functionality and leaves users without workaround.
- Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
- Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
- Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
- Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status