Skip to content

Conversation

@Fedik
Copy link
Member

@Fedik Fedik commented Jun 29, 2022

Summary of Changes

This is redo #37636
Create a _db without use of magic __get.

The magic get changes behavior of the object A LOT, example if any model have created a property "on fly" it crashes.
Like:

class FooBar{
  public function __get($name){ return null; }
}

$c = new FooBar;
$c->_cache[1] = 'foobar';

var_dump($c->_cache[1] ); // Will be NULL instead of "foobar"

That what happen in

// Convert to the \Joomla\CMS\Object\CMSObject before adding other data.
$properties = $table->getProperties(1);
$this->_cache[$pk] = ArrayHelper::toObject($properties, CMSObject::class);

Testing Instructions

Because it is an alternative fix, it still should work as before.
Addittionaly, please test any 3d component, wich use _db in its model.

Documentation Changes Required

As part of #37095

@Fedik
Copy link
Member Author

Fedik commented Jun 29, 2022

hm, how to ignore this in PHPCS?

Property name "$_db" should not be prefixed with an
underscore to indicate visibility

@richard67
Copy link
Member

richard67 commented Jun 29, 2022

hm, how to ignore this in PHPCS?

Property name "$_db" should not be prefixed with an
underscore to indicate visibility

@Fedik You can add your file to the list of exceptions here https://github.com/joomla/joomla-cms/blob/4.2-dev/ruleset.xml#L37 or you can put a comment at the top of your file at the same place as here https://github.com/joomla/joomla-cms/blob/4.2-dev/tests/Unit/bootstrap.php#L13 , but of course with another rule:

// phpcs:disable PSR2.Classes.PropertyDeclaration.Underscore

I would prefer to do it in the XML, but a recent discussion in my closed PR #38168 has shown that at least @HLeithner would prefer to do it in the PHP file but with an explaining comment why it's needed. @laoneo was also involved in the discussion. Maybe they can tell you in which of the 2 ways it shall be done.

@richard67
Copy link
Member

@Fedik Will this solve issue #38173 ? Or does it solve only a part?

@Fedik
Copy link
Member Author

Fedik commented Jun 29, 2022

Nope, there need separated fix.

@laoneo
Copy link
Member

laoneo commented Jun 30, 2022

There are a couple of core models which are still using _db like the back end plugins model.

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.

5 participants