Add Prefix, Suffix settings to Fraction fields

I agree this should be included. Fraction should essentially include all the same features that the core number fields provide. It is meant to be a replacement for the decimal field.

In Drupal 8 we may be able to simply extend the core number field type with our Fraction class. In D7 I think we would need to replicate the code from number.module. Probably not too difficult... I'm not sure if that will require a hook_update_N() to update existing installations.

In addition to "prefix" and "suffix", the core number module also has "min" and "max" settings for number fields. We should follow suit with all four settings. Updating this issue title and description to reflect that. Also, the Drupal 8 version is the canonical branch now, so we need to do it there first and then backport it to D7.

I won't have a chance to work on this immediately - working towards a project deadline currently... but it's good to have on the list.

Alright I couldn't resist... here's a patch for the 7.x-1.x branch. I haven't tested it at all - just copied code from the core number module.

I'd still like to commit a patch to the D8 version first, but since I had 7.x-1.x open it wasn't hard to do this.

I don't think we need an update hook for this - omitting the min, max, prefix, and suffix settings from a field shouldn't break anything. But we should test to make sure no errors or warnings show.

I went ahead and committed this to the 7.x-1.x branch.

Changing this issue to 8.x-2.x and "Patch (to be ported)".

@m.stenta I've given a stab to this and I would say that the biggest challenge here is that the precision/auto_precision of the field are actually happening in the widget, not sure if that would be better to carry it over to the field/instance settings, because that way we could move all validations to constraints and remove them from the form.

Otherwise, I don't think we can calculate the min/max for validation in the field settings as NumericBase expects, we should move everything to be widget settings instead.

Ah good points @pcambra - I think that makes perfect sense! I don't recall any good reason why it was in the widget...

I assume that will require an update hook to migrate the settings for existing sites, too, yes?

Given that we're in stable release, this probably could justify a 1.4 and would require some sort of update hook, I'll look into it.

On further review, given that the data schemas are a bit heterogeneous, for example validating decimal min/max on a Fraction widget with numerator/denominator would be poor UX, I've added separated properties for everything. I've also included some styling fixes ("new" array syntax basically) and ran the module through config inspector to add the missing keys.

Find the changes attached in a patch and also in https://github.com/mstenta/fraction/pull/1, happy to go either way, but I must say I kinda like the old-drupal ways too much :D, plus, we benefit from drupal-ci.

Idk how I downloaded the patch :), but here's the right one, I hope

@pcambra Great!! Would it be difficult to split this up into separate isolated commits? I'm all for the changes, but I like to be able to use Git history (eg line annotations in PHPStorm or "git blame" on command line) to trace specific changes, if needed in the future. It might be that these changes are a bit hard to tease apart - but if you can that would be great!

eg: maybe a first commit for the array syntax change, then a commit for this issue?

I'm curious about the new fraction.schema.yml file too - I'm not familiar with that (which just means I'm not fully up-to-speed on D8). Could you fill me in on how that works a bit?

happy to go either way, but I must say I kinda like the old-drupal ways too much :D, plus, we benefit from drupal-ci.

Agreed! The drupal-ci is a big reason for keeping farmOS issues in the drupal.org issue queue. I'd really like to incorporate automated testing into our development workflow from the beginning in the 8.x-2.x branch and use it more!

In cases where multiple commits are involved, making a branch that can be viewed as isolated changes is helpful for me. GitHub's fork (and online viewing) seems like the easiest way to do that (I don't believe we can fork projects on drupal.org, right?)

8.x-1.x: PHP 5.5 & MySQL 5.5, D8.9 Composer error. Unable to continue.

Are the tests failing because of drupal.org issues? Or something in this patch? Can't really tell... looks like maybe d.o needs to update something?

Thanks again @pcambra!

Hey @m.stenta, thanks for taking a look.

The array changes and such are done only in the files that I've touched (not a general coding styles thing), I'm not a big fan of having separate code styling issues (because they tend to linger and they're ugly to maintain) as long as the changes are delimited. However, if you prefer, we can do it the other way.

The only thing is that it would be handy for me if we could leave this one as it is, as I don't really have the changes separated atm. I'll take this into account for next patches.

I've been following the infra slack and there have been a few issues recently with the dispatcher (https://dispatcher.drupalci.org/), but I've changed the test to run in PHP 7, I think it will use different containers and that might be the issue, let's give it a bit of room because it's a bit busy currently.

EDIT: It just finished and we're green, so that was it, the PHP 5.5 images are kind of obsolete, so I think the default config in the module under "Automated testing" should be updated accordingly to test on PHP 7.

I think you're right, let's do feature branches for big changes and just patches for small things.

As far as I understand the schema files, those are to ensure that the data structure saved by the config entities such as typed data (fields), widgets, formatters and any kind of module config in general is saved with the correct data types, otherwise I think it will be considered just a string. It is important for numeric values to avoid funky casting and helps with translatability for labels, see https://www.drupal.org/docs/8/api/configuration-api/configuration-schema.... I think it's safer to have a schema, specially now that we have much more data on it (mins, maxes, prefixes, suffixes).
I thought about opening a different issue for schema changes (I can do that easily because it's basically one file), but most of the changes are related to this issue anyways...

I'll leave the decision of changing the issue status to you based on the above :)

The only thing is that it would be handy for me if we could leave this one as it is, as I don't really have the changes separated atm. I'll take this into account for next patches.

Sounds good - yea it seemed like it might be hard to separate.

The array changes and such are done only in the files that I've touched ... I'm not a big fan of having separate code styling issues

Yea I agree - we don't need a separate issue. Would you mind doing another quick commit that updates the array styles in the rest of the code, for consistency? I can merge that in as part of the same pull request, or separately, whatever's easier.

It just finished and we're green, so that was it, the PHP 5.5 images are kind of obsolete, so I think the default config in the module under "Automated testing" should be updated accordingly to test on PHP 7.

Ah great that makes sense! I updated the 8.x-1.x branch automated testing settings so that the default PHP version is 7. So all future tests for that branch should use that now. Thanks!

And thanks for the schema doc link! Reading up on it now... but yes I agree let's merge this in with these changes.

I think this is ready to be merged! Do you want to add one more commit to the GitHub branch for the array styling update before I do?

🎉🎉🎉

Let's merge this in and I'll do a separate issue on code styling

@pcambra: I'm testing this patch on my local D8 instance and something else just jumped out at me that I didn't notice before (sorry)...

In the "Fraction" widget settings, the patch adds "min" and "max" for both the numerator and denominator, requiring the user to specify the min/max numerator AND the min/max denominator. I don't think this is what we want... The desire is to be able to say "this number (represented as a fraction), should be between this minimum and maximum".

I'm not sure it would be useful in practice to define separate min/max for the numerator/denominator. For example, if you wanted to say that the fraction must be between 0 and 1, it wouldn't be possible.

The 7.x-1.x branch does not separate them - it just has a single "min" and "max" field, and then converts the fraction to a decimal during validation to check that it is between that range. I think we want to replicate that same behavior in the 8.x-1.x branch.

Does that complicate things? Sorry for the last minute change! Didn't see this until just now.

given that the data schemas are a bit heterogeneous, for example validating decimal min/max on a Fraction widget with numerator/denominator would be poor UX, I've added separated properties for everything

I see now I misunderstood this statement earlier - apologies for not bringing it up then.

Hmm... I'm also experiencing some PHP errors when I tried updating the module on a site that already has a Fraction field.

At first, I just updated to the new commit, and saw:

Error: Class 'Drupal\fraction\Plugin\Field\FieldType\Fraction' not found in Drupal\field\FieldStorageConfigStorage->mapFromStorageRecords() (line 166 of /var/www/html/core/modules/field/src/FieldStorageConfigStorage.php) #0 /var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php(193): Drupal\field\FieldStorageConfigStorage->mapFromStorageRecords(Array, Array) #1 /var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php(291): Drupal\Core\Config\Entity\ConfigEntityStorage->doLoadMultiple(Array) #2 /var/www/html/core/lib/Drupal/Core/Entity/EntityBase.php(541): Drupal\Core\Entity\EntityStorageBase->loadMultiple(Array) #3 /var/www/html/core/modules/field/field.module(181): Drupal\Core\Entity\EntityBase::loadMultiple(Array) #4 [internal function]: field_entity_field_storage_info(Object(Drupal\Core\Entity\ContentEntityType)) #5 /var/www/html/core/lib/Drupal/Core/Extension/ModuleHandler.php(392): call_user_func_array('field_entity_fi...', Array) #6 /var/www/html/core/lib/Drupal/Core/Entity/EntityFieldManager.php(570): Drupal\Core\Extension\ModuleHandler->invoke('field', 'entity_field_st...', Array) #7 /var/www/html/core/lib/Drupal/Core/Entity/EntityFieldManager.php(447): Drupal\Core\Entity\EntityFieldManager->buildFieldStorageDefinitions('node') #8 /var/www/html/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php(298): Drupal\Core\Entity\EntityFieldManager->getFieldStorageDefinitions('node') #9 /var/www/html/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php(129): Drupal\Core\Entity\EntityDefinitionUpdateManager->getChangeList() #10 /var/www/html/core/modules/system/system.install(762): Drupal\Core\Entity\EntityDefinitionUpdateManager->getChangeSummary() #11 [internal function]: system_requirements('runtime') #12 /var/www/html/core/lib/Drupal/Core/Extension/ModuleHandler.php(403): call_user_func_array('system_requirem...', Array) #13 /var/www/html/core/modules/system/src/SystemManager.php(112): Drupal\Core\Extension\ModuleHandler->invokeAll('requirements', Array) #14 /var/www/html/core/modules/system/src/SystemManager.php(96): Drupal\system\SystemManager->listRequirements() #15 /var/www/html/core/modules/system/src/Controller/SystemController.php(102): Drupal\system\SystemManager->checkRequirements() #16 [internal function]: Drupal\system\Controller\SystemController->overview('system.admin_co...') #17 /var/www/html/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array) #18 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(582): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() #19 /var/www/html/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure)) #20 /var/www/html/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) #21 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(151): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() #22 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1) #23 /var/www/html/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #24 /var/www/html/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #25 /var/www/html/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #26 /var/www/html/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true) #27 /var/www/html/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #28 /var/www/html/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(52): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #29 /var/www/html/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #30 /var/www/html/core/lib/Drupal/Core/DrupalKernel.php(693): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #31 /var/www/html/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request)) #32 {main}.

I went to /update.php and ran it, even though there weren't any updates. That fixed the error.

However, now when I try to edit the field settings for the fraction field, I get:

Error: Cannot call abstract method Drupal\Core\Field\WidgetInterface::formElement() in Drupal\fraction\Plugin\Field\FieldWidget\FractionDecimalWidget->formElement() (line 110 of /var/www/html/modules/fraction/src/Plugin/Field/FieldWidget/FractionDecimalWidget.php) #0 /var/www/html/core/lib/Drupal/Core/Field/WidgetBase.php(335): Drupal\fraction\Plugin\Field\FieldWidget\FractionDecimalWidget->formElement(Object(Drupal\Core\Field\FieldItemList), 0, Array, Array, Object(Drupal\Core\Form\FormState)) #1 /var/www/html/core/lib/Drupal/Core/Field/WidgetBase.php(202): Drupal\Core\Field\WidgetBase->formSingleElement(Object(Drupal\Core\Field\FieldItemList), 0, Array, Array, Object(Drupal\Core\Form\FormState)) #2 /var/www/html/core/lib/Drupal/Core/Field/WidgetBase.php(104): Drupal\Core\Field\WidgetBase->formMultipleElements(Object(Drupal\Core\Field\FieldItemList), Array, Object(Drupal\Core\Form\FormState)) #3 /var/www/html/core/lib/Drupal/Core/Field/FieldItemList.php(287): Drupal\Core\Field\WidgetBase->form(Object(Drupal\Core\Field\FieldItemList), Array, Object(Drupal\Core\Form\FormState)) #4 /var/www/html/core/modules/field_ui/src/Form/FieldConfigEditForm.php(127): Drupal\Core\Field\FieldItemList->defaultValuesForm(Array, Object(Drupal\Core\Form\FormState)) #5 /var/www/html/core/lib/Drupal/Core/Entity/EntityForm.php(117): Drupal\field_ui\Form\FieldConfigEditForm->form(Array, Object(Drupal\Core\Form\FormState)) #6 [internal function]: Drupal\Core\Entity\EntityForm->buildForm(Array, Object(Drupal\Core\Form\FormState)) #7 /var/www/html/core/lib/Drupal/Core/Form/FormBuilder.php(519): call_user_func_array(Array, Array) #8 /var/www/html/core/lib/Drupal/Core/Form/FormBuilder.php(276): Drupal\Core\Form\FormBuilder->retrieveForm('field_config_ed...', Object(Drupal\Core\Form\FormState)) #9 /var/www/html/core/lib/Drupal/Core/Controller/FormController.php(93): Drupal\Core\Form\FormBuilder->buildForm('field_config_ed...', Object(Drupal\Core\Form\FormState)) #10 [internal function]: Drupal\Core\Controller\FormController->getContentResult(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\RouteMatch)) #11 /var/www/html/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array) #12 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(582): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() #13 /var/www/html/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure)) #14 /var/www/html/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) #15 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(151): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() #16 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1) #17 /var/www/html/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #18 /var/www/html/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #19 /var/www/html/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #20 /var/www/html/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true) #21 /var/www/html/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #22 /var/www/html/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(52): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #23 /var/www/html/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #24 /var/www/html/core/lib/Drupal/Core/DrupalKernel.php(693): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #25 /var/www/html/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request)) #26 {main}.

Two PHP notices also occur with the new error:

Notice: Undefined index: prefix in Drupal\fraction\Plugin\Field\FieldType\FractionItem->fieldSettingsForm() (line 52 of /var/www/html/modules/fraction/src/Plugin/Field/FieldType/FractionItem.php)

Notice: Undefined index: suffix in Drupal\fraction\Plugin\Field\FieldType\FractionItem->fieldSettingsForm() (line 59 of /var/www/html/modules/fraction/src/Plugin/Field/FieldType/FractionItem.php)

I assume the errors are related to the changing of the Field class name?

Ah, thanks for checking that.

It is true that I've renamed the Fraction.php class to FractionItem.php on the fields as it seems to be the core pattern, I got confused myself while making changes on what Fraction class is what, so I think it's better DX, drush cr should solve the issue as you've seen with the update (I think that runs cache clear at the end).

The two errors you've found should be fixed on this new patch.

I'm looking into the min/max, as I'm not 100% sure on how friendly a validation of a fraction is with the calculated number?

(setting to NR to see if the errors go away, still work to do on this ticket)

The two errors you've found should be fixed on this new patch.

Great! I tested out the new commit you made to the GitHub branch, and that indeed clear up the error and notices.

drush cr should solve the issue as you've seen with the update (I think that runs cache clear at the end).

Yes, that seems to clear it up. I'm a bit worried that we'll get bug reports when users upgrade (if they don't clear cache or run updates - which of course they *should*).

It is true that I've renamed the Fraction.php class to FractionItem.php on the fields as it seems to be the core pattern, I got confused myself while making changes on what Fraction class is what, so I think it's better DX

Yea, I actually did name it FractionItem originally, and then renamed it in this commit: https://github.com/mstenta/fraction/commit/29a3c573295d319dc6f5612cec9ab...

I don't know why I did that. I agree it would be better to change it back to FractionItem for consistency with Drupal core. Maybe we should just add a note to the release notes that points out the need to clear cache immediately.

I'm looking into the min/max, as I'm not 100% sure on how friendly a validation of a fraction is with the calculated number?

Thanks!

Yea I think it's the only way we can do it (convert to decimal first and then run the comparison). The conversion can be done with "auto precision" so you don't need to know the precision setting when it's performed. That should be enough for validation.

Can we move the min/max settings to the field instance, to be consistent with number fields that are in core (eg: integer)?

@pcambra and I have looked at this together in more detail, and realized a flaw in my original min/max validation logic: it assumes that the comparison is being made on a base-10 number. It uses $precision = 0 and $auto_precision = TRUE, which only works with base-10 numbers. See: https://git.drupalcode.org/project/fraction/blob/8.x-1.x/src/Fraction.ph...

This means that the 7.x-1.x min/max logic has a bug, and may not work as expected if you are using the "fraction" widget (will work fine with the "decimal" widget).

With that in mind, and since @pcambra put a bunch of work into the other parts of this patch, I think we should separate the min/max validation to a new issue. And create a bug report issue for the existing 7.x-1.x code.

Renaming this to reflect that it is now only for the prefix/suffix settings...

Here's the WIP for min/max

Here is a new issue describing the bug in the 7.x-1.x branch: #3095745: Min/max validation doesn't work with fraction widget

Here is the new issue for adding min/max validation to the 8.x-1.x branch: #3095746: Add Min/Max settings to Fraction fields (dependent of solving the bug in 7.x-1.x first)

This should be the change without max/min as linked in the issues above

Merged the prefix/suffix changes. Min/max will be handled in #3095746: Add Min/Max settings to Fraction fields after #3095745: Min/max validation doesn't work with fraction widget is resolved.

Thanks again @pcambra!!!