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

Keyboard addRow array support #1356

Closed
wants to merge 1 commit into from
Closed

Keyboard addRow array support #1356

wants to merge 1 commit into from

Conversation

jyxjjj
Copy link
Contributor

@jyxjjj jyxjjj commented Aug 29, 2022

? !
Type improvement
BC Break no
Fixed issues null

Summary

make addRow both support button args and array of buttons.

I haven't test it.

I am sure it will not break this file but i don't know if there is any file else used this function like override or something, if so, it will break.

Please help me finish this.

@jyxjjj
Copy link
Contributor Author

jyxjjj commented Aug 29, 2022

Hummm, it seems checker broken when starting, instead of code check failed, i think it should be re-run.

@jyxjjj jyxjjj mentioned this pull request Aug 29, 2022
@TiiFuchs
Copy link
Member

Looks to me like you're completely ignoring $buttons here...
You may want to revise this.

@jyxjjj
Copy link
Contributor Author

jyxjjj commented Aug 29, 2022

No, if the first param is array, all button should be supplied in the first param, if not, func_get_args will got all the buttons. So the second param is actually useless.

If you want, there are two ways:

  1. remove , ...$buttons.
  2. i will submit another to add array_merge to use other params.

@TiiFuchs
Copy link
Member

Oh yeah, I've misread the func_get_args there...

The problem is though, that is_array($button) will alway be true, because a button is also just an array, an array of buttons is still an array. So the whole else part ist kind of never used in both scenarios...

@jyxjjj
Copy link
Contributor Author

jyxjjj commented Aug 29, 2022

Sorry but, i saw this

InlineKeyboardButton extends KeyboardButton
KeyboardButton extends Entity
Entity implements \JsonSerializable

My test code is:

class A implements \JsonSerializable
{
    public function jsonSerialize(): array
    {
        return [];
    }
}
echo is_array(new A()) ? '1' : '0';

Then it returns 0,

My mind: how could it be an array?


And i saw your code just now:

Request::sendMessage([
    'chat_id'      => 12345,
    'text'         => 'Test',
    'reply_markup' => [
        'inline_keyboard' => [
            [ // Row 1
              [ // Button 1 in 1st row
                'text'          => 'A',
                'callback_data' => 'A'
              ],
            ],
        ]
    ]
]);

Okay, then it is an array now
I never thought this way...
But, i think it still works,
Let me think, i will update later.

@TiiFuchs
Copy link
Member

Yeah, that's kind of a "problem" with the library for now...
A lot of stuff can be replaced with pure arrays...

We currently don't have a strong object coupling with keyboards...
The fluent-keyboard package tries to make this a bit sturdier.

@jyxjjj
Copy link
Contributor Author

jyxjjj commented Aug 29, 2022

Ok, i tried many ways, include this:

    public function addRow($buttons): Keyboard
    {
        if (is_array($buttons)) {
            $is_instance = true;
            foreach ($buttons as $button) {
                if (!($button instanceof KeyBoardButton)) {
                    $is_instance = false;
                }
            }
            if ($is_instance && ($new_row = $this->parseRow($buttons)) !== null) {
                $this->{$this->getKeyboardType()}[] = $new_row;
            }
        }
        if (($new_row = $this->parseRow(func_get_args())) !== null) {
            $this->{$this->getKeyboardType()}[] = $new_row;
        }
        return $this;
    }

And this:

    public function addRow(array $buttons): Keyboard
    {
        foreach ($buttons as $button) {
            if (!($button instanceof KeyboardButton)) {
                return $this;
            }
        }
        if (($new_row = $this->parseRow($buttons)) !== null) {
            $this->{$this->getKeyboardType()}[] = $new_row;
        }

        return $this;
    }

This:

    public function addRow($buttons): Keyboard
    {
        if (!is_array($buttons)) $buttons = [$buttons];
        foreach ($buttons as $button) {
            if (!($button instanceof KeyboardButton)) {
                if (($new_row = $this->parseRow(func_get_args())) !== null) {
                    $this->{$this->getKeyboardType()}[] = $new_row;
                }
                return $this;
            }
        }
        if (($new_row = $this->parseRow($buttons)) !== null) {
            $this->{$this->getKeyboardType()}[] = $new_row;
        }

        return $this;
    }

It is really hard to compatible with all situations,
Can we just make a BC break?

I submit this due to laravel's model's where function,
it can be used as:

->where('column', $data)
// or
->where('column', '=', $data)
// or
->where([
'column' => $data,
])

Any more opinion or advice?

@jyxjjj jyxjjj closed this by deleting the head repository Sep 14, 2022
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

2 participants