Problem/Motivation

Part of #3275851: [META] Fix PHP 8.2 dynamic property deprecations

This method (and connection opening) working because the $connection property is not defined on the class, so magic getter can attempt connection and save it into property which should be named diffent.

Testing Drupal\Tests\system\Functional\FileTransfer\FileTransferTest
.                                                                   1 / 1 (100%)

Time: 00:04.673, Memory: 4.00 MB

OK (1 test, 3 assertions)

Unsilenced deprecation notices (1)

  1x: Creation of dynamic property Drupal\Tests\system\Functional\FileTransfer\TestFileTransfer::$connection is deprecated
    1x in FileTransferTest::testJail from Drupal\Tests\system\Functional\FileTransfer

Steps to reproduce

Run Drupal\Tests\system\Functional\FileTransfer\FileTransferTest test on PHP 8.2 using cumulative patch #3295821-80: Ignore: patch testing issue for PHP 8.2 attributes
and at CI https://dispatcher.drupalci.org/job/drupal_patches/147180/testReport/jun...

Proposed resolution

Fix test and define missing property

Remaining tasks

patch/review/commit

User interface changes

no

API changes

no

Data model changes

no

Release notes snippet

no

CommentFileSizeAuthor
#9 3308744-8.patch3.39 KBandypost
#9 interdiff.txt596 bytesandypost
#7 3308744-6.patch2.89 KBandypost
#7 interdiff.txt1.21 KBandypost
#4 3308744-4.patch3.38 KBandypost
#3 3308744-3.patch3.38 KBandypost
#3 interdiff.txt565 bytesandypost
#2 3308744-2.patch3.37 KBandypost

Issue fork drupal-3308744

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Status: Active » Needs review
Related issues: -#3295821: Ignore: patch testing issue for PHP 8.2 attributes
StatusFileSize
new 3308744-2.patch3.37 KB

Fix from #3295821-80: Ignore: patch testing issue for PHP 8.2 attributes

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt565 bytes
new 3308744-3.patch3.38 KB

fix

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 3308744-4.patch3.38 KB

fix CS

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Needs work

Looks simple enough, but i have some questions. They might be overly defensive.

  1. +++ b/core/lib/Drupal/Core/FileTransfer/FileTransfer.php
@@ -41,6 +41,27 @@ abstract class FileTransfer {
+  protected $jail;

    If this is string, shouldn't the default be string? Can be null now. 

  2. +++ b/core/lib/Drupal/Core/FileTransfer/FileTransfer.php
@@ -41,6 +41,27 @@ abstract class FileTransfer {
+  private $chrootPath;

    Same here, it will start NULL right? Shouldn't it have a default? 

  3. +++ b/core/lib/Drupal/Core/FileTransfer/FileTransfer.php
@@ -41,6 +41,27 @@ abstract class FileTransfer {
+   * @var object|false

    see above. 

  4. +++ b/core/lib/Drupal/Core/FileTransfer/FileTransfer.php
@@ -93,12 +114,49 @@ public static function factory($jail, $settings) {
+  public function __set(string $name, $value): void {

    Won't this close off any other properties it might get or get set by any extending class? Is that part of the goal? 

  5. +++ b/core/lib/Drupal/Core/FileTransfer/FileTransfer.php
@@ -93,12 +114,49 @@ public static function factory($jail, $settings) {
+    return FALSE;

    So any property wouldn't be set? Even if it is? What if this class is extended? 

  6. +++ b/core/lib/Drupal/Core/FileTransfer/FileTransfer.php
@@ -93,12 +114,49 @@ public static function factory($jail, $settings) {
+    if ($name == 'connection') {

    Shouldn't this check for existing propeties and unset if they exist?

bbrala’s picture

bbrala
Location Netherlands
commented

Adding Slack discussion to this issue.

@andypost  [12:23 PM]@Björn Brala (bbrala) https://www.drupal.org/i/3308744 also tricky as I can't get why it works at all

Björn Brala (bbrala) Threading this up
Björn Brala (bbrala) Why wouldn't we just add protected $connection?
Björn Brala (bbrala) hmm
andypost Because absence of property makes this work((
Björn Brala (bbrala) would that fall through to __set?
andypost if we'll define this property then logic must change
Björn Brala (bbrala) so technically its treated as a public property then
andypost looking at usage it seems so
andypost maybe it needs someone from automatic updates innitiative
Björn Brala (bbrala) Has there been feedback already by commiters or something on implicit public properties?
andypost not yet, I filed it not a log ago
Björn Brala (bbrala) though you mightve run into that earlier :slightly_smiling_face:
andypost yes, just used to focus on other failures, so the patch is quick workaround
Björn Brala (bbrala) https://www.php.net/manual/en/language.oop5.overloading.php#object.get"The overloading methods are invoked when interacting with properties or methods that have not been declared or are not visible in the current scope. The rest of this section will use the terms inaccessible properties and inaccessible methods to refer to this combination of declaration and visibility."
Björn Brala (bbrala) so it could be a protected property then.
Björn Brala (bbrala) how would that break you think?
andypost I think we should get rid of this magic at all but it needs subsystem maintainer or commiter @catch @alexpott to chime in
Björn Brala (bbrala) @berdir said this issue wasn't really blocking. Is that true? Or is this a real failure we need to solve?
Björn Brala (bbrala) if we want to get rid of the magic functions, shouldn't it just be public properties then?
andypost no, he said it about "entity->original" issue and settings singleton
Björn Brala (bbrala) ahhhh sorry yes
Björn Brala (bbrala) My argument for a public property would be the fact it is already a public property right now. The only possible way that might break is if extending classes define that property as non-public, so that is maybe scary.
Björn Brala (bbrala) Then you probably just come back to what you built where you intercept the property to something else.
Björn Brala (bbrala) Also, the $connection property can probably be pretty much anything. All the different implementation just put something into it. Be is a ssh connection, ftp connecting or something else.
Björn Brala (bbrala) Cant find contrib usages.
andypost yes, no usage and I hope no usage in custom code.Moreover I hope this subsystem should gone in 10.x
Björn Brala (bbrala) Also, looking at the implementations, they indeed need the whole dance with __set to connect at the right time.
Björn Brala (bbrala) it can be refactored that the child classes just also connect, but your way also works and is closer to how it works right now
Björn Brala (bbrala) which means less chance for breakage
andypost I'm not sure in need for other magic methods implementation, probably it's useless (edited)
Björn Brala (bbrala) unset is used in update.authorize.inc
Björn Brala (bbrala) // The batch API uses a session, and since all the arguments are serialized
// and unserialized between requests, although the FileTransfer object itself
// will be reconstructed, the connection pointer itself will be lost. However,
// the FileTransfer object will still have the connection variable, even
// though the connection itself is now gone. So, although it's ugly, we have
// to unset the connection variable at this point so that the FileTransfer
// object will re-initiate the actual connection.
unset($filetransfer->connection);
Björn Brala (bbrala) (deleted) (edited)
Björn Brala (bbrala) I think this direction is good.
Björn Brala (bbrala) just fix the feedback for the properties
Björn Brala (bbrala) and perhaps for __isset and __unset add a parent call so you dont change how the class behaves
Björn Brala (bbrala) hmm
Björn Brala (bbrala) i think  your current magic isset and unset are right. Since if things exist, it wont fallback, and before we didnt handle that also.
Björn Brala (bbrala) So:
Fix the default values to something sane
Make sure the $connection is resource/boolean/string to allow for the different connection types
Björn Brala (bbrala) think then it would be done and i'd rtbc :x
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.21 KB
new 3308744-6.patch2.89 KB

Fixed doc-blocks and removed useless methods, it passes locally using skilldlabs/php:82 docker image with 8.2.0RC2

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Needs work

Unfortunately we do need the isset and unset magic methods, other code depends on that to know what to do.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt596 bytes
new 3308744-8.patch3.39 KB

yes, better to keep it, interdiff vs #4

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Reviewed & tested by the community

I've gone through this pretty extensively. I don't think we need a CR for this, the logic should be the same for the concrete classes.

  • catch committed 4d9f5fb on 10.0.x 
    Issue #3308744 by andypost, bbrala: Fix magic connection property access...
  • catch committed 7db1fcd on 10.1.x
    Issue #3308744 by andypost, bbrala: Fix magic connection property access...
  • catch committed 44451a4 on 9.5.x
    Issue #3308744 by andypost, bbrala: Fix magic connection property access...
catch’s picture

catch
he/him
Primary language English
commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

Probably a follow-up somewhere to not need all the magic, but this all might become irrelevant when automatic updates is further along.

Status: Fixed » Closed (fixed)

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

silverham’s picture

silverham commented

This fix is now causing a regression, as $object->connect() is called recursively and nested calls to PHP magic methods with the same parameters are ignored by PHP engine. So Drupal\Core\FileTransfer\FTPExtension->connect() no longer works. @see #3344877: [regression] FTPExtension class can no longer connect as of 9.5.x.

Notice: Undefined property: Drupal\Core\FileTransfer\FTPExtension::$connection in Drupal\Core\FileTransfer\FTPExtension->connect() (line 16 of core/lib/Drupal/Core/FileTransfer/FTPExtension.php).

alexpott made their first commit to this issue’s fork.