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

Declaration of Column::data does not compatible with Fluent::data #229

Closed
alberto-morais-oliveira-f opened this issue Nov 26, 2024 · 8 comments · Fixed by #230
Closed

Declaration of Column::data does not compatible with Fluent::data #229

alberto-morais-oliveira-f opened this issue Nov 26, 2024 · 8 comments · Fixed by #230

Comments

@alberto-morais-oliveira-f

Summary of problem or feature request

Error

Declaration of Yajra\DataTables\Html\Column::data(array|string $value): static must be compatible with Illuminate\Support\Fluent::data($key = null, $default = null)

Code snippet of problem

File vendor/yajra/laravel-datatables-html/src/Html/Column.php, function data (line 305 - 310) was the reason:

public function data(array|string $value): static
{
     $this->attributes['data'] = $value;

     return $this;
}

System details

Operating System: Linux Debian 12
PHP Version: 8.3
Laravel Version: laravel/framework ^11.34
Laravel-Datatables Version: yajra/laravel-datatables ^11.0

Solution

File vendor/yajra/laravel-datatables-html/src/Html/Column.php, change function data (line 305 - 310) to:

public function data($key = null, $default = null): static
{
    $this->attributes['data'] = $key;

    return $this;
}
@vipertecpro
Copy link

@yajra Can you please quickly look into this, this looks like very recently occurred. Here

https://github.com/laravel/framework/blame/11.x/src/Illuminate/Support/Fluent.php#L116

18 hours ago

Please help us asap

@eunir
Copy link

eunir commented Nov 27, 2024

Please help us asap

@yajra
Copy link
Owner

yajra commented Nov 28, 2024

The issue was due to a recent update in the Fluent class via laravel/framework#53665.

We will have to wait for it to be reverted or not before we make changes.

@stevebauman
Copy link

@yajra This can be fixed by simply making the definition compatible and overriding the visibility:

class Column extends Fluent
{
    // ...

    public function data(mixed $value, mixed $default = null): static
    {
        $this->attributes['data'] = $value;

        return $this;
    }
}

@yajra
Copy link
Owner

yajra commented Nov 28, 2024

@stevebauman Yes, it is the quickest solution as discussed in yajra/laravel-datatables#3201. But some devs might not like that idea.

@wmt-web-pruthvip
Copy link

For some releases, what if we stick with the current approach but add a deprecation warning?

/**
    * @deprecated 11.7.0 Use `setData` instead to set column data.
    *
    * Get data from the fluent instance.
    *
    * @param  array|string  $key
    * @return mixed
    */
public function data($key = null, $default = null)
{
    // Trigger a deprecation warning when this method is called
    trigger_error('The data() method is deprecated. Use setData() instead.', E_USER_DEPRECATED);

    return $this->setData($key);
}

/**
    * Set column data option value.
    *
    * @return $this
    *
    * @see https://datatables.net/reference/option/columns.data
    * @see https://datatables.net/manual/data/orthogonal-data
    */
public function setData(array|string $value): static
{
    $this->attributes['data'] = $value;

    return $this;
}

@yajra
Copy link
Owner

yajra commented Nov 28, 2024

I designed the Column builder to match the datatables.net (https://datatables.net/reference/option/columns) option. This way, it is easier to match and just call a magic method to set the desired options when looking at the official JS docs.

So it's a NO for me to go with deprecation. Thanks!

@wmt-web-pruthvip
Copy link

You have been managing this great package for a long time, so you have a better understanding of its usability. I have one short of suggestions that, We can remove the deprecation notice and use PHP DOC to notify the user about the logic update. However, we can make a new method that the user can directly use, like setData. And current, data method, which will redirect to parent if both arguments are provided. otherwise, it will be called setData internally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants