Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added default value to config #67

Merged
merged 4 commits into from
Mar 20, 2023
Merged

Conversation

daviddavo
Copy link
Contributor

@daviddavo daviddavo commented Feb 17, 2023

Related to #66

Furthermore, I'm trying to do an automatic unit test to test it, but I can't get it to work.

I'm trying to use $this->container->get('sherlockode_configuration.parameter_manager'); but it tells me Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "doctrine.orm.entity_manager".

I don't know how to define the service in a bundle, I haven't worked much with symfony, and just with applications.

About how to implement the default values, as I consider it, there are the following approaches:

  1. Modifying ParameterManager::loadParameters so it loads parameters with a default value even if they don't appear in the repository
  2. Modifying both get and getAll to consider the case a parameter is not in the repository.
  • For get, just check inside getUserValue if its null, and call the new $parameterDefinition->getDefaultValue() method.
  • For getAll, iterate through ConfigurationManager->getDefinedParameters, checking if its not already set in $this->data.

@daviddavo daviddavo marked this pull request as ready for review February 17, 2023 18:53
@daviddavo
Copy link
Contributor Author

I ended up following the second approach. I tested it by installing it in my application, because I couldn't come up with a solution for "integration test". Both get and getAll seem to return correct userValues, with correct types.

Now, getUserValue accepts a null value, and returns the default configured value if configured. Because of this, now we have to check for nullity before saving the userValue inside data

Form/Type/ParametersType.php Outdated Show resolved Hide resolved
Parameter/ParameterDefinition.php Outdated Show resolved Hide resolved
if (!is_null($uv)) {
$this->data[$path] = $uv;
return $this->data[$path];
}
Copy link
Member

Choose a reason for hiding this comment

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

it would be simpler to directly get the ParameterDefinition with $this->configurationManager->get($path); and read the default value (because here you already know you want the default value), and return it if not null

this make the getUserValue() changes not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... Now that I remember, I think I did it this way to transform the default value. Perhaps instead of overloading getUserValue I should create a new getDefaultValue($path) function. It would share most code with getUserValue, but the parts that use those functions would be easier to understand

Copy link
Contributor Author

@daviddavo daviddavo Mar 1, 2023

Choose a reason for hiding this comment

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

Oh, I think I understand now. I should get the raw value outside the getUserValue function, and then call getUserValue($path, $rawDefaultValue) to transform it.

Manager/ParameterManager.php Show resolved Hide resolved
- Changed variable names to camelCase
- Simplified getting default values in ParameterManager
- Reverted changes in ParameterManager->getUserValue
@daviddavo
Copy link
Contributor Author

I think I solved all issues, is there anything missing? Anything else I should do to improve it?

@juchi
Copy link
Member

juchi commented Mar 14, 2023

hi @daviddavo it seems good indeed !
Can you squash your commits so we can merge cleanly ?
Thanks

@daviddavo
Copy link
Contributor Author

I think you can also do it from GitHub online interface. There's a little arrow on the right of the green "Merge" button

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-commits

@Vowow Vowow merged commit 37b0cde into sherlockode:master Mar 20, 2023
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.

None yet

3 participants