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

Add the possibility to set the minimum number of decimals in the numb… #113

Open
wants to merge 14 commits into
base: 2.24.x
Choose a base branch
from

Conversation

MatthiasKuehneEllerhold
Copy link
Contributor

@MatthiasKuehneEllerhold MatthiasKuehneEllerhold commented Nov 13, 2023

Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC yes
QA no

Description

We have some user provided numbers that may have zero to
endless decimals. We want to display them in the user locale exactly as
provided.

If the user inputs "1", we want to display "1". If the user inputs "0,5432" (german notation!) we want to display "0,5432".

Sadly with the current (2.24.1) View-Helper we have to know beforehand how many decimals the user has used.

The underlying NumberFormatter-Class can do this: by
setting min-decimals to zero and max-decimals to PHP_INT_MAX (or some
large arbitrary number). The view-helper does not allow to set these 2
options separatly (yet). Thats what I added.

According to the github-template I should target the
"develop" branch. Sadly no branch named "develop" can be found in this
repository, so I've targeted the branch of the currently active minor
version. Please advice if this was the right approach.

Some commits are optional: adding type hint to variables and classes and may be discard if undesired.

@MatthiasKuehneEllerhold MatthiasKuehneEllerhold force-pushed the numberformatter_min_decimals_2_24 branch 2 times, most recently from b457165 to 5294806 Compare November 13, 2023 08:51
@MatthiasKuehneEllerhold
Copy link
Contributor Author

MatthiasKuehneEllerhold commented Nov 13, 2023

Added Unit-Tests and fixed CI (hopefully). DCO still says "signoff missing" although all commits are signed by my GPG key. What did I do wrong?
Additionally: Im not sure on how to fix the remaining psalm errors. Any hint please?

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

Thanks @MatthiasKuehneEllerhold - Generally speaking, whilst a desirable improvement, adding the native property/return/parameter types to existing props and methods all break BC. Because we're targeting a minor, they will have to be reverted and doc-blocks reinstated. Sorry!

Comment on lines 25 to 27
* The maximum number of decimals to use.
*/
protected ?int $maxDecimals = null;
Copy link
Member

Choose a reason for hiding this comment

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

This newly added property can be private - ideally, all of the properties in this class would be private, but it is not possible to make them so without breaking BC - this is because the class has not been marked as final and is therefore open to inheritance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ive added them as "protected" to keep consistent with the other attributes.
What I like about the zend framework then and laminas now is that I can overwrite and change classes and change behavior in subtle ways. Ideally I just overwrite a small piece of a class instead of copy and paste everything in a new class.
Thats why I'm personally in favour of making everything protected (and therefore accessable to inherited classes) instead of privating everything and forcing myself to copy-and-pasting whole classes.

src/View/Helper/NumberFormat.php Outdated Show resolved Hide resolved
src/View/Helper/NumberFormat.php Outdated Show resolved Hide resolved
src/View/Helper/NumberFormat.php Outdated Show resolved Hide resolved
src/View/Helper/NumberFormat.php Outdated Show resolved Hide resolved
src/View/Helper/NumberFormat.php Outdated Show resolved Hide resolved
src/View/Helper/NumberFormat.php Outdated Show resolved Hide resolved
test/View/Helper/NumberFormatTest.php Show resolved Hide resolved
src/View/Helper/NumberFormat.php Outdated Show resolved Hide resolved
Comment on lines 206 to 260
/**
* Get the maximum number of decimals.
*/
public function getMaxDecimals(): ?int
{
return $this->maxDecimals;
}

/**
* Get the minimum number of decimals.
*/
public function getMinDecimals(): ?int
{
return $this->minDecimals;
}
Copy link
Member

Choose a reason for hiding this comment

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

Are these getters necessary? It's better not to add new public methods when the effect of the new minDecimals property can be tested without verifying the value of the internal property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything else had a getter, so I added them.
Are they necessary though? I have a few situations where I get all internal state from an object, change it and then set the old state back. e. g. while rendering a view script I get the old locale from a lot of entities, set the desired locale, render my PHTML and then re-set the old locale.
Im not sure this situation would ever arise here though. But I erred on the side of caution and added them.

@MatthiasKuehneEllerhold
Copy link
Contributor Author

I could add another PR with all the type hint changes, this time targeting a more recent branch - if you like. All it would take is a git revert of ae1da38 (and possibly some more work in the other files ;-) ).

…er formatter

Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
Signed-off-by: Matthias Kühne <matthias.kuehne@ellerhold.de>
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