Selection: moved caching related functionality to separate classes by foxycode · Pull Request #185 · nette/database

foxycode · 2017-10-09T00:50:23Z

bug fix? no
new feature? yes
BC break? no

@dg Cache implementation is a long term problem in NDBT. While this is definitely not perfect, it's a starting point from which I would like to continue making NDBT code more clear, which wil help in future development. I spent two days with this, so I hope you appreciate it. Also, if you agree, I can maintain this repo as we spoken on NettCamp. There is lot for review and merge.

dg · 2017-10-09T16:02:07Z

Thanks for pull request. If I understand it, this is a pure refactoring that separates the code into new classes but does not change the functionality?

foxycode · 2017-10-09T17:16:35Z

@dg Yes, exactly :)

dg · 2017-10-09T17:25:23Z

src/Database/Table/ColumnAccessCache.php

+	}
+
+
+	public function setGeneralCacheKey(?string $key)


Because it is targeted to 7.1, you can use return type void.

dg · 2017-10-09T17:28:03Z

src/Database/Table/ColumnAccessCache.php

+
+
+	/**
+	 * @param mixed $accessedColumns


Please don't use property name in @param. I know that PhpStorm appends it automatically, but better is to be consistent with rest of code.

Btw is it really mixed or array?

It is really mixed

dg · 2017-10-09T17:28:56Z

src/Database/Table/ColumnAccessCache.php

+
+	/**
+	 * Loads cache of previous accessed columns and returns it.
+	 * @return array|bool


When it returns bool?

This is just move from original class. I wasn't sure so I reather left it as is.

There are more variables that use null and bool to distinguish some states. I would like to fix this in second run.

It seems that array_keys() can return only array, or not?

Yes, and it passes tests, fixed.

dg · 2017-10-09T17:29:34Z

src/Database/Table/ColumnAccessCache.php

+
+
+	/**
+	 * @param mixed $observeCache


mixed or bool?

Here iare stored Selection instances.

foxycode · 2017-10-09T17:36:13Z

Rest fixed

dg · 2017-10-09T17:54:47Z

src/Database/Table/ColumnAccessCache.php

+
+
+	/**
+	 * @param mixed


$previousAccessedColumns has typehint array and here is mixed

My bad. I copied properties as they were. Fixed on all properties.

dg · 2017-10-09T17:55:13Z

src/Database/Table/ColumnAccessCache.php

+	/**
+	 * @param mixed
+	 */
+	public function setAccessedColumns($accessedColumns): void


$accessedColumns has typehint array and here is mixed

dg · 2017-10-09T17:55:33Z

src/Database/Table/ColumnAccessCache.php

+
+
+	/**
+	 * @return mixed


$accessedColumns has typehint array and here is mixed

dg · 2017-10-09T17:56:05Z

src/Database/Table/ColumnAccessCache.php

+
+
+	/**
+	 * @param mixed


$observeCache has typehint bool and here is mixed

Fixed, allowed only Selection.

foxycode · 2017-10-10T14:59:00Z

@dg Anything else I can do so you merge it?

dg · 2017-10-10T15:34:25Z

src/Database/Table/ColumnAccessCache.php

+	/** @var string */
+	protected $specificCacheKey;
+
+	/** @var mixed of touched columns */


What means mixed of? Is it array or not?

There was lot of mess arround mixed properties, so I get rid of them and simplify code in separate commit. Let me know what you think.

dg · 2017-10-11T10:58:55Z

src/Database/Table/ReferenceCache.php

+
+
+	/**
+	 * @return mixed


It seems according to the code that it is array|null, or not? For getReferenced too.

Yes, you are right

dg · 2017-10-11T11:00:17Z

src/Database/Table/ColumnAccessCache.php

+
+	public function setAccessedColumn(string $key, bool $value): void
+	{
+		if (!$this->cache) {


Just such a stupid thing, maybe positive condition can be used here and in setAccessedColumns

if ($this->cache) { $this->accessedColumns[$key] = $value; } ```

Fixed on both places.

dg · 2017-10-11T11:00:30Z

Now it look much better, thanks!

dg · 2017-10-11T11:21:27Z

src/Database/Table/ReferenceCache.php

+	/**
+	 * @return array|null
+	 */
+	public function &getReferencing(string $generalCacheKey)


Sry, now I realized that it can be written native as &getReferencing(string $generalCacheKey): ?array

Yes, I should realize that too. Fixed

dg · 2017-10-11T11:44:29Z

Do you want to have two commits, one with mixed and the other without mixed properties?

foxycode · 2017-10-11T11:55:35Z

@dg No, it was just for easier code review, squashed.

It would be good to resolve other PR's before I get further. Like this one, which is not needed now: #161 Will you do that or should I take care of that for you?

dg · 2017-10-11T12:18:35Z

Thanks, great!

#185)" This reverts commit a561254.

dg reviewed Oct 9, 2017

View reviewed changes

dg reviewed Oct 10, 2017

View reviewed changes

dg reviewed Oct 11, 2017

View reviewed changes

Selection: moved caching related functionality to separate classes

fad84f8

dg merged commit a561254 into nette:master Oct 11, 2017

dg added a commit that referenced this pull request Apr 16, 2019

Revert "Table: moved caching related functionality to separate classes (

8b6230d

#185)" This reverts commit a561254.

Uh oh!

Conversation

foxycode commented Oct 9, 2017

Uh oh!

dg commented Oct 9, 2017

Uh oh!

foxycode commented Oct 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foxycode commented Oct 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foxycode commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dg commented Oct 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dg commented Oct 11, 2017

Uh oh!

foxycode commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dg commented Oct 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

foxycode commented Oct 11, 2017 •

edited

Loading