The Feeds module cannot currently map data to Fraction fields. The Fraction module needs to provide targets that can be mapped to.

CommentFileSizeAuthor
#22 2892160-21.patch7.12 KBm.stenta
#21 interdiff-2892160-20.txt639 bytesm.stenta
#21 2892160-21.patch7.13 KBm.stenta
#20 2892160-20.patch6.95 KBpcambra
#20 interdiff-2892160-19.txt1.08 KBpcambra
#19 2892160-15-interdiff.txt3.58 KBpcambra
#19 2892160-19.patch6.94 KBpcambra
#15 2892160-15.patch8.72 KBpcambra
#9 fraction_feeds-2892160-9.patch4.59 KBm.stenta
#6 fraction_feeds-2892160-6.patch3.83 KBm.stenta
#3 fraction_feeds-2892160-2.patch1.71 KBm.stenta
#2 fraction_feeds-2892160-1.patch1.57 KBm.stenta

Comments

m.stenta created an issue. See original summary.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Title: Provide targets for Feeds integration » Provide Fraction targets for Feeds integration
Status: Active » Needs review
StatusFileSize
new fraction_feeds-2892160-1.patch1.57 KB

Attached is a patch that implements hook_feeds_processor_targets() to provide mapping targets for fraction fields.

Currently this only accepts a single "decimal" value, which automatically gets converted to numerator and denominator by the normal Fraction field code.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
StatusFileSize
new fraction_feeds-2892160-2.patch1.71 KB

Previous patch didn't work. Need to set numerator and denominator specifically, rather than just passing in the decimal. Updated patch attached.

web226’s picture

web226 commented

Very useful and timely patch for me. Thanks! However I'm having a little trouble. I have fractions to display so in my feed source data I converted them to decimal for import. i.e 1/2 --> 0.5 - This example resulted in the imported node displaying 5/10. So my question is, what should the source data look like? Thanks

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

@web226 - Ah yea... what's happening is you are converting your fraction (1/2) to decimal (0.5) and then the Fraction module is converting that back to a fraction... but when the module converts to decimal it always does so with a base-10 denominator (ie: 10, 100, 1000, etc). So that's why it is ending up with 5/10 instead of 1/2.

It might make sense to have the module reduce the fraction, so 5/10 would automatically become 1/2.

But... it seems like the actual goal you are trying to achieve is to import fractions with specific numerators and denominators. So instead of importing "0.5" as the decimal equivalent, you actually want to import "1" as the numerator and "2" as the denominator.

As an example, say you wanted to import the fraction "4/8". If we convert it to decimal and back again, AND reduce it, then the end result would be "1/2:. But if you wanted to specifically store "4/8" then that doesn't work.

So, it sounds like we need additional mapping targets in the Feeds integration. In addition to the current target, which expects a decimal value, we should also provide a "numerator" target and a "denominator" target, so that they can be imported separately.

What do you think?

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
StatusFileSize
new fraction_feeds-2892160-6.patch3.83 KB

Here is a new patch that includes numerator and denominator targets. It seems to be working, but could use more testing.

It handles null values gracefully: If the numerator is omitted it will default to 0. If the denominator is omitted it will default to 1. If both are omitted, no field value will be saved. And it is not possible to import an empty denominator.

Give this a try and let me know what you think, or if you find any bugs...

web226’s picture

web226 commented

Many thanks @m.stenta #5 is exactly what I am trying to achieve: import specific numerator and denominator. Thanks for the patch I will test today.

web226’s picture

web226 commented

I tested the patch in #6 and it works perfectly when the numerator and denominator are in separate columns in the source csv data file.

Do you know of a way I can have the fraction in a single column of the source data i.e. '1/4' and then have this split by the feeds mapper into numerator and denominator to be saved into the node field. It doesn't seem possible to do this with feeds tamper as a single source mapped to two targets.

Also, as a feature request is it possible to add the standard 'Prefix & Suffix' to the field? Thanks!

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
StatusFileSize
new fraction_feeds-2892160-9.patch4.59 KB

Try this patch - I added a fourth target, which accepts a fraction in the form "x/y". It pulls the numerator and denominator out by splitting on the slash. I wrote it but I haven't tested it...

web226’s picture

web226 commented

The patch in #9 works perfectly. Thank you so much, I'm extremely grateful for your work!

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Thanks for testing @web226! I'm going to mark this as RTBC.

Ideally we should make a patch for the D8 version of Fraction first, but since this one is ready to go I may go ahead and commit to to the 7.x-1.x branch and then change this issue to 8.x-2.x so we can work on the D8 version. I haven't tested Feeds on D8 yet, and it seems like it's still in development.

  • m.stenta committed cce5e9c on 7.x-1.x 
    Issue #2892160 by m.stenta: Provide Fraction targets for Feeds...
m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I committed this to the 7.x-1.x branch (a slight variation on the previous patch: moved the Feeds code to a fraction.feeds.inc include file to keep the module file tidy).

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

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Related issues: +#3018566: Provide Fraction targets for Feeds 8.x integration

An initial patch was provided for 8.x-1.x here: #3018566: Provide Fraction targets for Feeds 8.x integration. It only provided the "fraction" target, however. So in order to have parity with the 7.x-1.x branch, we also need a decimal target, as well as separate numerator and denominator targets.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Assigned: Unassigned » pcambra
Status: Patch (to be ported) » Needs review
StatusFileSize
new 2892160-15.patch8.72 KB

Here's a patch for review, the changes made are the following:

- Renamed Fraction.php to FractionTarget.php as we did for the field because it's clearer and less confusing to DX.
- Enabled Fraction.php (the main one) to be initialised with empty numerator & denominator so we can create a fraction from decimal with no information on those.
- Added decimal, numerator and denominator target options (not 100% sure what the use case of numerator and denominator by themselves, so feel free to point me in the right direction there)
- Tests

Regarding Fraction.php, I am a bit confused on the loose typing there, I think we should aim to strong type it, for example, numerator/denominator accepting ints, getters returning ints instead of strings, decimals as floats, etc, but that's a tale for another issue, I guess. I can open a follow up for this if you want to explore it.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Needs review » Postponed
Related issues: +#3097527: Add feeds as a dependency for our tests

I think tests are failing due to #3097527: Add feeds as a dependency for our tests

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Status: Postponed » Needs review

Merged #3097527: Add feeds as a dependency for our tests. Requeuing the test...

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

Tests passed -- hooray!

Reviewing in more detail now...

Enabled Fraction.php (the main one) to be initialised with empty numerator & denominator so we can create a fraction from decimal with no information on those.

Can you help me understand where this is needed?

Added decimal, numerator and denominator target options (not 100% sure what the use case of numerator and denominator by themselves, so feel free to point me in the right direction there)

Yea maybe we don't actually need separate targets for Numerator and Denominator anymore. Originally I added them in 7.x-1.x because @web226 needed them, but then we decided to add a "Fraction" target which let you import both in the same field, separated by slash.

I agree it's sort of weird to import them separately - and it's actually more complicated because we need to ensure both are present when the fraction is finally saved. We could simplify by removing those targets. Can you update the patch to remove them?

Regarding Fraction.php, I am a bit confused on the loose typing there, I think we should aim to strong type it, for example, numerator/denominator accepting ints, getters returning ints instead of strings, decimals as floats, etc, but that's a tale for another issue, I guess. I can open a follow up for this if you want to explore it.

Please do! It is worth thinking this through.

We use BC Math (when available) to perform fraction arithmetic using strings instead of floats - to avoid float precision issues. https://www.php.net/manual/en/intro.bc.php

I see that some values are being cast to (int) in this patch. I'm not sure we want to do that... I'd have to give it more thought but I'm inclined to say we should follow what we did in the 7.x-1.x patch as closely as possible. What do you think?

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
StatusFileSize
new 2892160-19.patch6.94 KB
new 2892160-15-interdiff.txt3.58 KB

Enabled Fraction.php (the main one) to be initialised with empty numerator & denominator so we can create a fraction from decimal with no information on those.

Can you help me understand where this is needed?

This is so we can do:

      case 'decimal':
        $fraction = new Fraction();
        $fraction->fromDecimal($item);
        $values['numerator'] = $fraction->getNumerator();
        $values['denominator'] = $fraction->getDenominator();
        break;

Otherwise we need a different way to create fraction objects, like allowing an optional decimal

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
StatusFileSize
new interdiff-2892160-19.txt1.08 KB
new 2892160-20.patch6.95 KB
m.stenta’s picture

m.stenta
he/him
Primary language English
commented
StatusFileSize
new 2892160-21.patch7.13 KB
new interdiff-2892160-20.txt639 bytes

Otherwise we need a different way to create fraction objects, like allowing an optional decimal

Ah that makes sense. In that case, I'm think it would be better to default to a fraction of 0/1 instead of null/null. This is what the fraction() helper function does. Attached is a new patch with that minor change (and interdiff).

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
StatusFileSize
new 2892160-21.patch7.12 KB

Oops, reverse patch. :-)

  • m.stenta committed e05ac40 on 8.x-1.x authored by pcambra 
    Issue #2892160 by m.stenta, pcambra: Provide Fraction targets for Feeds...
m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Status: Needs review » Fixed

This is working really well! I merged this and also pushed a few commits for handling unusual values (like negative denominators and empty strings), along with some more tests for those kinds of things.

Thanks @pcambra!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.