Regression: Currency No longer working

[ragingdave]

• Laravel Version: 6.16.0
• Nova Version: 2.11.1
• PHP Version: 7.3.13
• Database Driver & Version: mysql
• Operating System and Version: CentOS 7
• Browser type and version: Chrome 80
• Reproduction Repository: https://github.com/###/###

Description:

In updating to the latest version from 2.10.1, I find that my app is now broken even after running the upgrade steps including running a composer update. (also tried to rm -rf my vendor directory and re-install fully without a lock file). This is caused by the following error:

The Symfony\Component\Intl\NumberFormatter\NumberFormatter::setAttribute() method's argument $attr value 2 behavior is not implemented. The available attributes are: FRACTION_DIGITS, GROUPING_USED, ROUNDING_MODE. Please install the "intl" extension for full localization capabilities. {"userId":1,"exception":"[object] (Symfony\Component\Intl\Exception\MethodArgumentValueNotImplementedException(code: 0): The Symfony\Component\Intl\NumberFormatter\NumberFormatter::setAttribute() method's argument $attr value 2 behavior is not implemented. The available attributes are: FRACTION_DIGITS, GROUPING_USED, ROUNDING_MODE. Please install the \"intl\" extension for full localization capabilities.

This was of course not happening prior to updating and based on the symfony/intl package being required I would not have expected this to happen. This maybe is somewhat related to #2369 , although that sounds like a different issue, but he does mention that it's still happening though... It also looks to be caused by https://github.com/laravel/nova/pull/747 which adds in the Money usage as well as the symfony/intl package so perhaps the installations of php in the dockerized containers just automatically contain the intl extension, but this appears to be a rather hard breaking change.

I should also note that I am in the en locale so even the symfony docs don't list the extension as being required either while in that locale.

Steps To Reproduce:

Load up a resource with a Currency field....see the whole page break.

[ragingdave]

Additional follow up to this, I installed the intl extension just to be able to keep working but hit another new bug. A data field value that previously caused no error (a value of null) on load of the resource view when using the Currency field now throws an exception and blows up the page as well with error:

The given value "" does not represent a valid number. {"userId":1,"exception":"[object] (Brick\Math\Exception\NumberFormatException(code: 0): The given value \"\" does not represent a valid number.

That same page under 2.10.1 loaded up appropriately with that same value being passed to the currency field. So this is also breaking previously working data sets with the new Currency fixes.

[Zen0x7]

Additional follow up to this, I installed the intl extension just to be able to keep working but hit another new bug. A data field value that previously caused no error (a value of null) on load of the resource view when using the Currency field now throws an exception and blows up the page as well with error:

The given value "" does not represent a valid number. {"userId":1,"exception":"[object] (Brick\Math\Exception\NumberFormatException(code: 0): The given value "" does not represent a valid number.

That same page under 2.10.1 loaded up appropriately with that same value being passed to the currency field. So this is also breaking previously working data sets with the new Currency fixes.

You can use observers, specifically retrieve method. With a simple is null or empty method you can check the property and replace with a valid number. But this "patch" apply only on show view and on specific model instance which doesnt have property value.

[ragingdave]

I'm not sure what you mean by using observers as the specific issue is needing a new php extension installed just for this to function and a value that was previously allowed and viewable in the previous version of the Currency field (null) no longer is handled appropriately as it was before. That's why I updated the title to be regression, because the feature parity between the previous version and this new version for the Currency field isn't matched.

Specifically if the suggestion is adding a model level observer to replace null with a number, that's not really a solution as you are suggesting showing a value which currently doesn't exist in the db. I would think the correct solution would be to be able to handle a null value rather than cause a breaking change for data that previously worked.

[Zen0x7]

I agree with you. Thanks for the explanation 👍

[DSpeichert]

It certainly looks like the format() function has been removed from the Currency class, while the documentation still lists it:

You may specify the display format using the format method; otherwise, the %i format will be used

https://nova.laravel.com/docs/2.0/resources/fields.html#currency-field

[ragingdave]

@DSpeichert I'm not sure that has anything to do with this issue specifically, but good to note I guess...Perhaps submitting a PR to the nova docs would be more helpful :)

@demency Is there something in the works to address this regression?

[jbrooksuk]

If anyone is able to, it'd be great if someone could test out https://github.com/laravel/nova/pull/818 for me, please?

[jbrooksuk]

I've made quite a few changes to the field here.

[ragingdave]

@jbrooksuk While that looks cool to be able to support minor units.....and specifically require ext-intl too 🎉 ....It still doesn't seem to resolve the issue around handling null values as the check used isNullValue doesn't properly handle a null value. Perhaps this is just difference of interpretation but having a function called isNullValue that doesn't return if the value passed is null or not, and will return false even if the value is null, but the field isn't nullable.....seems wrong to me and is obfuscating the true nature of that function.

It seems that at least for the time being, adding nullable to the Currency field seems to resolve my issue, but I still think this whole update to Currency field was far too loose with comparing feature parity from the old to the new. Unless of course the fix added to reinforce the requirement of nullable was intended then I guess my case is just collateral damage in that respect

[DSpeichert]

@ragingdave At this point, I'm not sure what changed. I saw it at the same time when you saw, meaning it's probably caused by https://github.com/laravel/nova/pull/747.

However, I don't have the issue you're having. I will open a separate issue but it will be about bringing format() back, not removing it from docs.

[gldrenthe89]

I have the same issue. i have to add ->nullable() to every currency field. wich is not correct

[barclaymichael]

Same issue, I ended up using https://github.com/vyuldashev/nova-money-field

[Payetus]

Having same issue. Using laravel nova v3.6.0 with php 7.4.2

class TitleFields
{
    /**
     * Get the pivot fields for the relationship.
     *
     * @return array
     */
    public function __invoke()
    {
         return [ -Text::make('Nombre', 'name'),
            MorphMany::make('Paquetes', 'packages', Package::class),
            MorphToMany::make('Extras')->fields(function(){
                return [

                    Currency::make('Precio', 'price'),
                    Boolean::make('Optional')
                ];
            })
        ];
    }
}

This Fails with the same error.

class TitleFields
{
    /**
     * Get the pivot fields for the relationship.
     *
     * @return array
     */
    public function __invoke()
    {
         return [ -Text::make('Nombre', 'name'),
            MorphMany::make('Paquetes', 'packages', Package::class),
            MorphToMany::make('Extras')->fields(function(){
                return [

                    Number::make('Precio', 'price'),
                    Boolean::make('Optional')
                ];
            })
        ];
    }
}

While this works fine

[zealouscreations]

I'm having this exact same issue with nova v2.12.0 (upgraded from v2.10.1) with PHP 7.2.24

Is there any solution to this? I've set ->nullable() on the Currency fields but this is also happening on Number fields and setting them nullable does not help.

[arimolzer]

Also experiencing this issue on Nova 3.8.2 / Laravel 7.25.0 / PHP 7.4

Really sucks that I have to use the Number field type instead.

[pleone]

Any update on this ?

[crynobone]

Hello everyone, I'm trying to replicate the issues and found the following:

Using the following:

        return [
            ID::make()->sortable(),
            Text::make('Name')->rules('required', 'max:255'),
            Text::make('Email')->sortable(),
            Currency::make('Balance'),
        ];

class Issue2372AddBalanceToUsersTable extends Migration
{
    public function up()
    {
        Schema::table('users', function ($table) {
            $table->integer('balance');
        });
    }
}

• Currency field work fine if the balance is not null.
• Currency field will throw exception "The given value "" does not represent a valid number. " when balance is null.

My initial expectation as of now is that balance should either be nullable or has a default value (can be set from Model::$attributes), which should solve the error but I would like to hear our use case to understand more about the issue.

Perhaps this is just difference of interpretation but having a function called isNullValue that doesn't return if the value passed is null or not, and will return false even if the value is null, but the field isn't nullable.....seems wrong to me and is obfuscating the true nature of that function.

AFAIK the isNullValue()explicitly require nullable() at the moment. Would like to hear your requirement where the "currency" value can be null but is not nullable.

---

Also it seem that there's conflicting info based on feedback by @zealouscreations and @arimolzer suggesting Number field working fine (and doesn't work). If there any other scenario that cause the same error but on Number please open a new issue so I can create regression tests based on that use case.

[zealouscreations]

@davidhemphill - Why was this closed? This is still an issue for me. I downgraded back to v2.10.1 because of this bug, and would appreciate this being fixed so that I can upgrade.

After @crynobone's comment, I attempted v2.12.0 again and now, even with adding ->nullable() to Currency fields the error still persists. And I even tried adding a default value via the Model::$attributes as @crynobone suggested, but that did not help either. The Number fields appear to be working fine - with or without ->nullable(). So, as of my latest testing, this is only an issue with Currency.

Here is what I have that will reproduce this error:

create table `event`
(
    cost double null
);

<?php

namespace App;

class Event extends Model
{
    protected $table = 'event';
}

<?php

namespace App\Nova;

class Event extends Resource
{
    public static $model = 'App\Event';

    public function fields(Request $request) {
        return [
            Currency::make('Cost', 'cost'),
        ];
    }
}

This, IMO, should work - as it is - with no errors, but it doesn't. I'm requesting that this please be reopened. Unless there was a PR that fixed this and I missed it?

Thanks.

[crynobone]

Hello @zealouscreations

The code has been fixed with v3.12.0. Unfortunately, Nova 2.x is no longer supported and you should try upgrading to 3.x.

[zealouscreations]

@crynobone That would require me to pay for that. So this is quite disappointing that something that I've purchased is broken and in order for me to get it to work I have to pay for an upgrade.

[crynobone]

That would require me to pay for that.

AFAIK Laravel Nova 3 is still under the Orion License Series, so you should be able to upgrade without any cost. I'll check this with management about this today.

[davidhemphill]

@zealouscreations All current licenses include v1-3.

[mxm1070]

@crynobone @davidhemphill This doesn't seem to have been fixed. I just updated to version 3.12.1, republished all the files, and it still throws the error if I remove the explicit nullable(). So is the accepted solution to just always use nullable() going forward?

[crynobone]

@mxm1070 Thank you for the report.

Yes there been some missing changes during the initial PR causing it to be reverted before v3.12.0 release. Send a new PR laravel/nova#1008

---

Temporary fix for now you could do:

<?php

namespace App\Nova\Fields;

class Currency extends \Laravel\Nova\Fields\Currency
{
    /**
     * Check value for null value.
     *
     * @param  mixed $value
     * @return bool
     */
    protected function isNullValue($value)
    {
        if (is_null($value)) {
            return true;
        }

        return parent::isNullValue($value);
    }
}

[kgp43]

Any update on this? Problem still exist it seems...

[crynobone]

Hi there, this issue has been marked as fixed and done based on above description, reproducing code and test done on our side. If you still have a problem please create a new issue with full description on how to reproduce the issue based on your use case.