Problem/Motivation

#2304909: Relax requirement for @file when using OO Class or Interface per file updates our @file docblock standards to require no @file docblock and nothing between the <?php and namespace for autoloaded classes, interfaces, and traits. The proxy class generator is not compliant with this standard.

Proposed resolution

Update the proxy class generator to:

  • Not add a file docblock.
  • Use the standard section of the class docblock to add docs for how it is generated.

Remaining tasks

  • Once the proxy generation is fixed, we also need to change the generated classes appropriately.

User interface changes

N/A

API changes

Internal changes only. Addition of one token to the ProxyBuilder.

Data model changes

N/A

CommentFileSizeAuthor
#3 interdiff-proxy.txt1.17 KBxjm
#3 proxy-2676836-3.patch1.78 KBxjm
proxy.patch1.77 KBxjm

Comments

xjm created an issue. See original summary.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented 
  1. +++ b/core/lib/Drupal/Core/Command/GenerateProxyClassCommand.php
@@ -71,20 +71,9 @@ protected function execute(InputInterface $input, OutputInterface $output) {
+      $proxy_class_generation_ = "This file was generated via php core/scripts/generate-proxy-class.php '$class_name' '$namespace_root";

    Shouldn't this be $proxy_class_generation_docs ? Maybe I'm missing something but it seems wrong... 

  2. +++ b/core/lib/Drupal/Core/Command/GenerateProxyClassCommand.php
@@ -71,20 +71,9 @@ protected function execute(InputInterface $input, OutputInterface $output) {
+$file_string = str_replace(['{{ proxy_class_name }}', '{{ proxy_class_generation_docs }}'], [$proxy_class_name, $proxy_class_generation_docs], $proxy_class_string);

    Weird indentation here. It seems like it should go about 6 spaces in?

xjm’s picture

xjm
she/her
Primary language English
commented
StatusFileSize
new proxy-2676836-3.patch1.78 KB
new interdiff-proxy.txt1.17 KB

Oops, attached tries to fix those two things.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

That smells better. ;) I'll leave it to you to verify that the script actually works, and then I guess also run the script and put updated proxy class files themselves into the patch.

The last submitted patch, proxy.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: proxy-2676836-3.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes

Attached attempts to implement these changes, but still needs to be tested. I am missing something because when I try to run the script I get:

You need book module installed for it.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.2.x-dev » 8.1.x-dev

This change is needed for #2304909: Relax requirement for @file when using OO Class or Interface per file, so moving back to 8.1.x.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2676836.10.patch16.65 KB

I created a duplicate issue which fixes all the proxy classes and adds a flag to not check the files for coding standards - which is correct because these files are using PSR1 and PSR2 since they are being generated by Symfony code.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community 
  1. +++ b/core/lib/Drupal/Core/Command/GenerateProxyClassCommand.php
@@ -73,11 +73,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 <?php
-
-/**
- * @file
- * Contains \{{ proxy_class_name }}.
- */
+// @codingStandardsIgnoreFile

    Ah we don't have any tests for this file, I guess this is fine. 

  2. +++ b/core/lib/Drupal/Core/ProxyClass/Lock/PersistentDatabaseLockBackend.php
@@ -106,6 +102,14 @@ public function releaseAll($lock_id = NULL)
          */
+        public function schemaDefinition()
+        {
+            return $this->lazyLoadItself()->schemaDefinition();
+        }
+

    We don't actually need this because of the __call implementation, right?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Re #11.1 Well these files don't obey our standards so i think we should ignore them
Re #11.2 What __call implementation - the reason this is in this patch is because (not unreasonably) I just used to regenerate the proxies - we've added methods to the class being proxied but forgot to rerun it so... here we are.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +rc target

Since this is non-disruptive and a blocker for a scheduled change for the RC, I am going to consider this an RC target.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

  • catch committed 8c4ec60 on 8.2.x 
    Issue #2676836 by xjm, alexpott: Update proxy class generator for new @...
drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Needs work » Fixed

Correcting issue status, see #2698635: Issue statuses changing by themselves?.

Status: Fixed » Closed (fixed)

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