laravel / prompts

Beautiful and user-friendly forms for your command-line PHP applications.
https://laravel.com/docs/prompts
MIT License
533 stars 94 forks source link

Select Prompt does not work correctly with associative array #119

Closed amshehzad closed 8 months ago

amshehzad commented 8 months ago

Laravel Prompts Version

0.1.16

Laravel Version

10.46.0

PHP Version

8.2.0

Operating System & Version

Windows 11

Terminal Application

PowerShell

Description

If we use associate array with numeric keys, it does not return the key of selected option but the value itself.

Steps To Reproduce

$role = select(
    label: 'What role should the user have?',
    options: [
        1 => 'Member',
        2 => 'Contributor',
        3 => 'Owner',
    ],
    default: 1
);

dd($role); // 'Member' instead of 1
driesvints commented 8 months ago

That's the way this works. Integer keys aren't supported here I'm afraid. Maybe try '1' to see if that works?

amshehzad commented 8 months ago

@driesvints Yes I tried both ways, changed the numeric order but it's the same

driesvints commented 8 months ago

I think this is only meant for string based keys, sorry.

amshehzad commented 8 months ago

Ah, but it's not mentioned in the docs though https://laravel.com/docs/10.x/prompts#select

I think this is pretty common usecase, do you think it might be added down the road? I might try a PR if you think this can be added

yolcuiskender commented 7 months ago

First, thanks to everyone for your hard work on this project. It's been incredibly useful.

I do think that @amshehzad is underlining a fair point.

I’ve encountered a situation regarding the select() prompt’s behavior with numerical arrays. Currently, the method treats all arrays the same, defaulting to text values for lists, which might not suit all use cases. For example, when working with indices from a database, numerical array support would be more intuitive and flexible.

I believe a simple enhancement could greatly improve this functionality without affecting existing codebases. My proposal is to introduce an optional parameter in the SelectPrompt constructor. This change would allow developers to explicitly opt for numerical index handling, broadening the use case and enhancing usability.

Here’s a conceptual snippet to illustrate the idea:

public function __construct(
    public string $label,
    array|Collection $options,
    public int|string|null $default = null,
    ...
    public bool $force_numeric_index = false // New optional parameter
) {

The value() implementation could be something like:

if (array_is_list($this->options)) {
    if ($this->force_numeric_index === true) {
        return $this->highlighted;
    } else {
        return $this->options[$this->highlighted] ?? null;
    }
} else {
    return array_keys($this->options)[$this->highlighted];
}

This approach should keep things backward compatible, make testing straightforward, and bring numerical array support.

I'm eager to contribute by working on this and can submit a PR if we agree on the direction. As always, I'm open to any feedback or suggestions on this proposal. Together, we can enhance prompts for everyone's benefit.

jessarcher commented 7 months ago

The select Prompt will already return the key in a numerically-keyed associative array as long as the array is not a list (i.e. the keys aren't consecutive integers starting from 0). For database indexes this would normally be fine as they typically start from 1 rather than 0.

In the example provided in the original issue, Prompts will return 1 rather than "Member" because the array is not a list.

image

The real issue is that when Prompts is used in Laravel on Windows, it falls back to Symfony, and the Symfony prompt does not follow the same behaviour:

image

The Symfony logic appears to be that if all keys are numeric (can be strings, non-consecutive, etc.) then it will return the label. As soon as you add a non-numeric key, it will always return the key for all options.

@timacdonald and I have come up with a potential workaround for this that wraps the labels in stringable objects containing the key and label. Symfony renders these objects fine and returns the object so we can retrieve the key when appropriate. I just need to test it with validation, etc.

yolcuiskender commented 7 months ago

Thank you for explaining in detail.

I understand the behavior you've described, and it clarifies the mechanism behind the select Prompt's handling of numerically-keyed associative arrays. I didn't know about the different behaviour of a Laravel environment on Windows. This was an aspect I hadn't encountered in my initial investigation, and it significantly clarifies the situation.

Your proposed workaround with @timacdonald, involving stringable objects that encapsulate both the key and label, sounds promising. It appears to offer a clever solution to the Symfony behavior - which I assume we all agree - should be consistent across all operating systems.

Regarding the handling of arrays with indices starting from 0, I'd like to illustrate why I believe my enhancement proposal is necessary with a specific use case:

When I am working with arrays of items identified by names—which may not be unique—I find it essential to rely on their indices for a unique identifier. This is because PHP automatically assigns numerical indices to the array, effectively making it a list that also represents the order of items. In situations where it's important to present a selection to the user, accurately identifying each item by its index is crucial.

Although I understand that the current implementation of the library can accommodate indices beginning from 1, this capability and its implications are not immediately obvious from the existing documentation, leading to potential confusion. My proposed change seeks to directly address this by explicitly facilitating the handling of lists, thus enhancing usability and ensuring backward compatibility.

To further support my proposal, I have prepared several tests that would all pass successfully should the enhancement be implemented. This addition aims to demonstrate the practical benefits and applicability of the proposed change.

it('Array Test: Associative array with consecutive numerical index starting from 1', function () {
    Prompt::fake([Key::DOWN, Key::ENTER]);

    $options = [
        1 => 'Red',
        2 => 'Green',
        3 => 'Blue',
    ];

    $result = select(
        label: 'What is your favorite color?',
        options: $options
    );

    expect($result)->toBe(2);
});

it('Array test: Associative array with non-consecutive numerical index', function () {
    Prompt::fake([Key::DOWN, Key::ENTER]);

    $options = [
        1 => 'Red',
        3 => 'Green',
        5 => 'Blue',
    ];

    $result = select(
        label: 'What is your favorite color?',
        options: $options
    );

    expect($result)->toBe(3);
});

it('Array test: Associative array with mixed indexes', function () {
    Prompt::fake([Key::DOWN, Key::ENTER]);

    $options = [
        1 => 'Red',
        3 => 'Green',
        5 => 'Blue',
        'foo' => 'Yellow',
    ];

    $result = select(
        label: 'What is your favorite color?',
        options: $options
    );

    expect($result)->toBe(3);
});

it('Array test: Associative array with string indexes', function () {
    Prompt::fake([Key::DOWN, Key::ENTER]);

    $options = [
        '0' => 'Red',
        '1' => 'Green',
        '2' => 'Blue',
        '3' => 'Yellow',
    ];

    $result = select(
        label: 'What is your favorite color?',
        options: $options
    );

    expect($result)->toBe(1);
});

it('Array Test: Associative array with consecutive numerical index', function () {
    Prompt::fake([Key::DOWN, Key::ENTER]);

    $options = [
        0 => 'Red',
        1 => 'Green',
        2 => 'Blue',
    ];

    $result = select(
        label: 'What is your favorite color?',
        options: $options
    );

    expect($result)->toBe(1);
});

it('Array Test: Existing use case', function () {
    Prompt::fake([Key::DOWN, Key::ENTER]);

    $options = [
        0 => 'Item 1',
        1 => 'Item 2',
        2 => 'Item 2',
        3 => 'Item 2',
    ];

    $result = select(
        label: 'Which item would you like to remove from the list?',
        options: $options
    );

    expect($result)->toBe(1);
});

image

Inclusion of these examples aims to illustrate the practical benefits and potential improvements my proposal could bring to the project. I am very much looking forward to hearing your thoughts on these suggestions and examples.

Collaboratively, I believe we can find the best path forward that accommodates a variety of use cases and strengthens the library for all users.

jessarcher commented 7 months ago

Can you please help me understand the real-world use case where you need to pass a list (i.e. consecutive integer keys starting from 0) and return the selected key rather than the selected value?

yolcuiskender commented 7 months ago

I am currently developing an internal tool focused on reading and editing YAML configuration files. This tool aims to simplify structure manipulation for enhanced usability. For illustrative purposes, here’s a simplified example of how blocks are defined in an array for ease of demonstration. Typically, in a production environment, these blocks are directly loaded from YAML files.

blocks:
  - "Text Block"
  - "Image Block"
  - "Text Block"
  - "Text Block"
info('Simplified example with blocks defined in an array. Normally, blocks are loaded from a YAML file.');

$temp_block_names = [
    'Text Block',
    'Image Block',
    'Text Block',
    'Text Block',
];

// Example function call showcasing the proposed improvement
$selected_block_index = select(
    label: 'Delete a block',
    options: $temp_block_names,
    force_numeric_index: true, // Proposed flag to ensure numeric indexing
);

print_r('Deleted Index: ' . $selected_block_index . "\n\n");

For this array, PHP will assign numerical indexes starting from 0, effectively creating a list. My proposed solution (the force_numeric_index flag) makes the select function able to work with these types of arrays too. My solution is ready, works with the tests specified in my previous comments, and is essentially ready to go. I am happy to submit a PR.

yolcuiskender commented 7 months ago

I would like to kindly remind you of this issue.

Thank you