Fix cache() return value checks.

[sfadschm]

Closes #334.

cache($key) always returns the value read from cache or null.
The old !cache()-check will evaluate true for valid but empty cached values, e.g., [], causing unnecessary database calls.

[MGatner]

A test case for this would be awesome.

[MGatner]

Roave test failure is unrelated. Might need to look into how to handle fork Actions for Docker workflows.

[sfadschm]

I was thinking about that, but don't really know how to.
2 ideas which might work:

1. Is there an assertion to test whether a method calls the database or not?
2. We could check whether the timestamp of an "empty" cached property ([]) changes during the read operation of the same element.

Any other ideas or preferences?

[MGatner]

@sfadschm I don't think it needs to be that specific. I think ideally we test it bidirectionally:

1. Create a User in a Group yet seed the cache with [] - assert user groups are []
2. Create a User with no Groups yet seed the cache with a group - assert user groups equal that group

[sfadschm]

Sounds good! Will add that tomorrow.

[sfadschm]

Should the same tests also be added for getGroupsForUser and getPermissionsForUser?

[MGatner]

That would be great, both in case of fringe errors but also for coverage.

[sfadschm]

Tests are in and should be working. Not sure if they would be more efficient with the Faker approach, but I opted for manual definition of arrays. 😄

I noticed that for the GroupModel, users and groups are always stored in the cache as complete objects and then are decompiled by the user entity methods. In the PermissionModel, permissions are directly cached as $permissionId -> $permissionName. Is that intended and has some pratical relevance?

[sfadschm]

Another note:
There is some issues with CS regarding alignment and such. I chose not to fix that within the current PR, but maybe we can turn on the new CI4 php-cs-fixer along (or after) the static analysis PR?

[MGatner]

Is that intended and has some pratical relevance?

I doubt it. There is much about this library not standardized.

maybe we can turn on the new CI4 php-cs-fixer

We can make this a priority. I've been waiting on the framework one to be done for us to copy but we could make our own. @paulbalandan made me a great base for some modules I could probably adjust for here.

[sfadschm]

Curious to see that, too 😆
I currently have a git synced version of CI4 dev-develop on another folder on my drive and have php-cs-fixer load its configuration from there in my modules/project. Does work, but not really pretty...

[MGatner]

Of course! Check out my toolkit: https://github.com/tattersoftware/codeigniter4-tools/tree/develop

Particularly pay attention to the CS Config file in root and src/Standard.php. It uses Paul's Nexus CS Config library, all credit to him for the great work on this.

[sfadschm]

Nexus really eases the configuration.
However, the toolkit and nexus do still not pull the current utils/PhpCsFixer/CodeIgniter4.php standard from the CI4 repo but rather define a duplicate definition, right?

[MGatner]

@sfadschm That's correct. This is something we will look into once the framework rules are complete, applied, and released.

[paulbalandan]

I see that I'm pinged here. 😄 Sorry, not able to work on remaining fixers as work has been pressing lately. Will be able to start again later this week.

@sfadschm yes, cs-config does not pull the configuration as it serves only as a factory for configurations. I think my last comment on allowing Utils\ namespace on dist versions may work.

@MGatner unrelated to the conversation but I've been seeing you activating beStrictAboutCoversAnnotation in phpunit.xml.dist here and in your modules but not activating also the forceCoversAnnotation attribute. For a stricter testing, I recommend enabling also that so beStrictAboutCoversAnnotation will also take effect. See Specifying Covered Code Parts

[lonnieezell]

@MGatner is this one good to merge or are we waiting?

[MGatner]

Yes, this is good to go!