Refactor Model::getFields #691

georgehristov · 2020-08-05T14:06:54Z

Introduce constants to declare Model field filter presets
Adds Model::FIELD_FILTER_PERSIST filter preset
Adds Model::FIELD_FILTER_ONLY_FIELDS filter preset
uses new presets to remove duplicate code
Provide the possibility to filter fields by Closure

src/Model.php

mvorisek · 2020-08-05T14:49:39Z

Provides possibility to filter fields by Closure

I do not get this - we should get all and filter then, what is the advantage of your approach?

georgehristov · 2020-08-05T15:04:01Z

This is also true. Main point was to introduce the 'persisting' preset to avoid duplicate code.
And streamline the code a bit with the ifs and buts.
Passing a Closure as filter is a side-effect

src/Model.php

mvorisek · 2020-08-18T09:26:53Z

src/Model.php

@@ -625,15 +625,21 @@ public function isDirty(string $field): bool
     *
     * @return Field[]
     */
-    public function getFields($filters = null): array
+    public function getFields($filters = null, $onlyFields = null): array


bool $onlyFields?

but why to null by default?

To keep BC

$model->getFields() returns all fields historically
$model->getFields('system') returns system fields in onlyFields

So if $filter argument set $onlyFields defaults to true

mvorisek · 2020-08-18T09:39:44Z

src/Model.php

-        } elseif (is_string($filter)) {
-            $filter = [$filter];
+        if ($filters === null) {
+            return $onlyFields ? $this->getFields(self::FIELD_FILTER_ONLY_FIELDS) : $this->fields;


no need to recurse, just set $filter...

mvorisek · 2020-08-18T09:40:22Z

src/Model.php

-        return array_filter($this->fields, function (Field $field, $name) use ($filter) {
-            // do not return fields outside of "only_fields" scope
-            if ($this->only_fields && !in_array($name, $this->only_fields, true)) {
+        $onlyFields = $onlyFields ?? true;


remove this then

mvorisek · 2020-08-18T09:41:10Z

src/Model.php

+                return $field->isEditable();
+            case self::FIELD_FILTER_VISIBLE:
+                return $field->isVisible();
+            case self::FIELD_FILTER_ONLY_FIELDS:


we should not need FIELD_FILTER_ONLY_FIELDS then

georgehristov · 2020-08-18T10:14:39Z

I have another idea:
Presently filters are in first argument as array and considered OR.

We can introduce multiple arguments which will be filters merged with AND.

So $model->getFields('system', 'only_fields') will return fields system AND only fields
So $model->getFields(['system', 'only_fields']) will return fields system OR only fields

It will not be BC though I believe.

mvorisek · 2020-08-18T10:27:55Z

I have another idea:
Presently filters are in first argument as array and considered OR.

I like minimalistic APIs. We extend Model more and more for BC... And the questions is what this more and more code provides? Does it really help and reduce the maintanence requirements? If yes, go ahead!

mvorisek

I like to deduplicate the code.

However, I am not sure if we should mix very unrelated attributes together and offer partial "not" constants

romaninsh

Good improvement.

Do address feedback from @mvorisek

mvorisek · 2023-09-07T14:54:15Z

TODO rebase on develop, it is based on #690, only the last 10 commits are relevant (others are from the mentioned PR) - be3cb50...d6307b3

…sting' filter preset

…hrows exception

georgehristov added the waitingForAnotherPR label Aug 5, 2020

georgehristov force-pushed the feature/model-fields-filtering-persisting branch from 1132e47 to 945d6b9 Compare August 5, 2020 14:19

georgehristov marked this pull request as ready for review August 5, 2020 14:21

georgehristov requested review from DarkSide666 and mvorisek August 5, 2020 14:35

mvorisek reviewed Aug 5, 2020

View reviewed changes