Skip to content

Conversation

@thirsch
Copy link
Collaborator

@thirsch thirsch commented Dec 15, 2022

And here is the doctrine1 part of the 8.2 support.

For every missing property, I've tried to keep the style used in the surrounding classes. Some are written "each prop per line", others are separated by comma, as in sf1. My primary goal was to not change too much of existing things but make it compatible to 8.2.

Here is the 8.2 support branch for sf1: FriendsOfSymfony1/symfony1#274

@thirsch thirsch marked this pull request as ready for review December 15, 2022 10:19
* @author Nicolas Bérard-Nault <[email protected]>
* @author Jonathan H. Wage <[email protected]>
*/
#[\AllowDynamicProperties]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method setOption($key, $value) is using dynamic properties:

/**
 * setOption
 *
 * @param string $key
 * @param string $value
 * @return void
 */
public function setOption($key, $value)
{
    $name = 'set' . Doctrine_Inflector::classify($key);

    if (method_exists($this, $name)) {
        $this->$name($value);
    } else {
        $key = '_' . $key;
        $this->$key = $value;
    }
}

public function testSetSequenceName()
{
$this->objTable->sequenceName = 'test-seq';
$this->objTable->setOption('sequenceName', 'test-seq');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test might have been wrong for a long time. I created the following example to demonstrate, that without using setOption, this test accidentally created a new public property instead of setting the value in the options array:

<?php

class X {
    private $storage = [];
    
    public function __construct() {
        $this->storage['foo'] = 'bar';
    }
    
    public function __get($name) {
        var_dump('#### magic getter!');
        return $this->storage[$name];
    }
    
    public function getStorage($name) {
        return $this->storage[$name];
    }
}


$x = new X();
var_dump($x->foo);                  // Returns the value of X::$storage['foo']
var_dump($x->getStorage('foo'));    // Returns the value of X::$storage['foo']
$x->foo = "baz";                    // Creates a new dynamic property $foo
var_dump($x->foo);                  // Returns the value of the dynamic property $foo
var_dump($x->getStorage('foo'));    // Returns the value of X::$storage['foo']

Copy link
Member

@thePanz thePanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, not sure why the CI did not run 🤔
Can you rebase this PR @thirsch ?

@thirsch thirsch force-pushed the feature/php-8.2-changes branch from 8b455cd to 4e3c6c0 Compare January 9, 2023 21:07
Copy link
Member

@thePanz thePanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

  • would it be possible to keep the git commit messages consistent? (php 8.2 >)
  • Could you please rebase (merged another PR) again?

@thirsch thirsch force-pushed the feature/php-8.2-changes branch from 4e3c6c0 to f338825 Compare January 10, 2023 12:31
…d in case of clear() and moved the property _pendingJoinConditions fom Doctrine_Query up to Doctrine_Query_Abstract in the hierarchy.
@thirsch thirsch force-pushed the feature/php-8.2-changes branch from f338825 to 0241c0b Compare January 10, 2023 12:34
@thirsch
Copy link
Collaborator Author

thirsch commented Jan 10, 2023

Thanks!

  • would it be possible to keep the git commit messages consistent? (php 8.2 >)
  • Could you please rebase (merged another PR) again?

Sure, commit messages are fixed and branch is rebased.

@thePanz thePanz merged commit 0ee6863 into FriendsOfSymfony1:master Jan 10, 2023
@thirsch thirsch deleted the feature/php-8.2-changes branch February 10, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants