I am splitting off a side issue from

#2908886: Split off schema management out of schema.inc

From a new service method SchemaVersionHandler::setInstalledVersion() I want to enforce a return type of void.

  public function setInstalledVersion(string $module, string $version): void {
    $this->keyValue->get('system.schema')->set($module, $version);
    $this->currentVersions[$module] = $version;
  }

this is not possible with the current Builder, the output ALWAYS returns what it is was given

        public function setInstalledVersion(string $module, string $version): void
        {
            return $this->lazyLoadItself()->setInstalledVersion($module, $version);
        }

I am going to modify builder to just call the method without explicit return

PS

Just for context the proxyBuilder was recently semi-forced to start accounting for type to avoid conflicts with Symfony 5
https://www.drupal.org/project/drupal/issues/3125234

CommentFileSizeAuthor
#10 3143173-10.patch2.39 KBmartin107
#10 interdiff-3143173-4-10.txt970 bytesmartin107
#8 Fail-3143173.patch2.98 KBmartin107
#4 interdiff-3143173-2-4.txt1.26 KBmartin107
#4 3143173.4.patch2.55 KBmartin107
#2 3143173-2.patch2.55 KBmartin107

Comments

martin107 created an issue. See original summary.

martin107’s picture

martin107 commented
Assigned: martin107 » Unassigned
StatusFileSize
new 3143173-2.patch2.55 KB

The fix is simple.

The existing test strategy was comprehensive .. making is a dream to add to the tests. :)

martin107’s picture

martin107 commented
Status: Active » Needs review
martin107’s picture

martin107 commented
StatusFileSize
new 3143173.4.patch2.55 KB
new interdiff-3143173-2-4.txt1.26 KB

Resolved CS issues.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Parent issue: #2908886: Split off schema management out of schema.inc » #3050720: [Meta] Implement strict typing in existing code
Related issues: +#2908886: Split off schema management out of schema.inc
andypost’s picture

andypost
he/him
Primary language Russian
commented
Category: Task » Bug report

It looks more like bug, please add test as fail.patch

Moreover it looks backportable to 8.9 because required php version supports it already

EDIT 7.1 https://wiki.php.net/rfc/void_return_type

martin107’s picture

martin107 commented
Title: ProxyBuilder does not respect return type void » Followup: ProxyBuilder compatibility with Symfony 5 - needs to handle voids correctly.

Thank you for adding that perspective ... andypost++

In general, we have no control over what Symfony5 will hand us.

This really is a follow up to a bug I added ...[ hangs head in shame. ]

I will add a patch tonight.

martin107’s picture

martin107 commented
StatusFileSize
new Fail-3143173.patch2.98 KB

Here is the smallest patch which shows the failure.

I have added a new class -- AboutToFail.php

core/lib/Drupal/Core/File/MimeType/AboutToFail.php

I then ran the appropriate script to trigger the ProxyBuilder into generating the proxy class.

php core/scripts/generate-proxy-class.php 'Drupal\Core\File\MimeType\AboutToFail' "core/lib/Drupal/Core"

The result is

core/lib/Drupal/Core/ProxyClass/File/MimeType/AboutToFail.php

As can be seen the generated file contains a syntax error with message...

"A void function cannot return a value."

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Good spot, thanks for fixing this! I agree this can't go back to 8.9 because void return type is not valid in PHP 7.0.

  1. +++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php
@@ -295,12 +295,20 @@ protected function buildParameter(\ReflectionParameter $parameter) {
+
+    $returnsVoid = $reflection_method->getReturnType() ? $reflection_method->getReturnType()->getName() === 'void' : FALSE;

    Extra blank line at start of method. Variable name should be $returns_void. hasReturnType() could be used for the first part. 

  2. +++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php
@@ -295,12 +295,20 @@ protected function buildParameter(\ReflectionParameter $parameter) {
+      if ($returnsVoid) {

    Or we could just roll the variable into here? e.g.

    if ($reflection_method->hasReturnType() && $reflection_method->getReturnType()->getName() === 'void') {
martin107’s picture

martin107 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-3143173-4-10.txt970 bytes
new 3143173-10.patch2.39 KB

Longwave - thanks for coming back and reviewing part 2 of this little thing.

Fixed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Looks good now, a simple fix and the test coverage is straightforward.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.1.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 4c29b0788c to 9.1.x and b425254cc7 to 9.0.x. Thanks!

Backported to 9.0.x to keep things consistent.

  • alexpott committed 4c29b07 on 9.1.x 
    Issue #3143173 by martin107, andypost, longwave: Followup: ProxyBuilder...

  • alexpott committed b425254 on 9.0.x 
    Issue #3143173 by martin107, andypost, longwave: Followup: ProxyBuilder...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.