Problem/Motivation

The proxybuilder is our approach to improve the speed by not initialization services, where its not needed.

Sadly it has a bug. Let's assume the following structure of code:


interface InterfaceA {}
interface InterfaceB extends InterfaceA {}
class ExampleClass implements InterfaceB {}

Sadly reflection returns both interfaces so the resulting proxy class looks like:

class Proxy_class_ExampleClass implements InterfaceB, InterfaceA

which is invalid PHP.

There is also an additional bug:

We also have to protect ourselves against including a proxy for multiple services using the same class.

Proposed resolution

Filter out parent interfaces.
Maybe some reflection gurus have better ideas, would be wonderful.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug, because the proxy builder produces invalid PHP
Issue priority Major, because it might block the usage of a proxy for some classses
Disruption No disruption, just internal fixes.
CommentFileSizeAuthor
#7 interdiff.txt328 bytesdawehner
#7 2408357-7.patch6.02 KBdawehner
#5 interdiff.txt1.67 KBdawehner
#5 2408357-5.patch6.06 KBdawehner
#1 2408357-1.patch4.27 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
2408357-1.patch4.27 KB

Alright, here is a patch, but its just sad that you have to do it.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue summary: View changes

We also have to protect ourselves against including a proxy for multiple services using the same class.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Related issues: +#2408371: Proxies of module interfaces don't work

Here is another issue which came up.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Needs work

Looks ready to me, except for a bunch of nitpicks (sorry…).

  1. +++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php
@@ -48,11 +48,29 @@ public function build($class_name) {
+    // php error.

    s/php/PHP/ 

  2. +++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php
@@ -48,11 +48,29 @@ public function build($class_name) {
+    // parents is also in the list and include it, in case.
+    // In order to avoid inc

    Some unfinished phrases here. 

  3. +++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php
@@ -48,11 +48,29 @@ public function build($class_name) {
+        // Exclude all parents from the list of implemented interface of the

    s/interface/interfaces/ 

  4. +++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyDumper.php
@@ -15,6 +15,8 @@
+  protected $buildClasses = [];

    Missing docs.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
2408357-5.patch6.06 KB
interdiff.txt1.67 KB

Thank you for your review.

Some unfinished phrases here.

Fixed.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php
@@ -1,4 +1,4 @@
-<?php
+<?PHP

I don't think this was intentional :) Can be fixed on commit.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
2408357-7.patch6.02 KB
interdiff.txt328 bytes

Ha.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +SprintWeekend2015

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed f12134b and pushed to 8.0.x. Thanks!

  • alexpott committed f12134b on 8.0.x 
    Issue #2408357 by dawehner: The ProxyBuilder includes parent interfaces...

Status: Fixed » Closed (fixed)

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