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

Custom variables: list or map from php #641

Open
uuf6429 opened this issue Jan 13, 2023 · 5 comments
Open

Custom variables: list or map from php #641

uuf6429 opened this issue Jan 13, 2023 · 5 comments
Milestone

Comments

@uuf6429
Copy link

uuf6429 commented Jan 13, 2023

First of all, I don't understand why ScssPhp\ValueConverter::fromPhp assumes scalar types are purely from PHP (correct) but array types are potentially Scss values (unexpected for me, bordering to wrong). Especially when one has to look at the code to figure that out.

if (is_array($value) && isset($value[0]) && \in_array($value[0], [Type::T_NULL, Type::T_COLOR, Type::T_KEYWORD, Type::T_LIST, Type::T_MAP, Type::T_STRING])) {


Secondly, the function signature takes in "mixed" type - nowhere does it say anything about objects or arrays not being supported.


Thirdly, this functionality is a very common use case (when it comes to custom variables), so it really should be implemented. 😅

I tried the following approach but it didn't work:

class MyClass {
    // ....

    private function scssFromPhp(mixed $value): mixed
    {
        if ($value === null || is_scalar($value)) {
            return ScssPhp\ValueConverter::fromPhp($value);
        }

        if (is_array($value) && array_is_list($value)) {
            return [ScssPhp\Type::T_LIST, '', array_map($this->scssFromPhp(...), $value)];
        }

        if (is_array($value) || $value instanceof stdClass) {
            $value = (array)$value;
            return [
                ScssPhp\Type::T_MAP,
                array_map($this->scssFromPhp(...), array_keys($value)),
                array_map($this->scssFromPhp(...), array_values($value))
            ];
        }

        throw new InvalidArgumentException('...'); // error message should detail exactly where the conversion failed and why
    }
}

I don't have much knowledge about these types' structure to be sure about that code, would be nice if someone could figure out what's wrong.

Seems the code does work 🤔 maybe it could be added to the implementation though..

@stof
Copy link
Member

stof commented Jan 13, 2023

The hard part is that we already support passing an already converted value there (due to some legacy behavior in the library), and those are represented as an array.

@uuf6429
Copy link
Author

uuf6429 commented Jan 13, 2023

Right, I ended up write a value translator class (scss <-> php, both directions).

Maybe I'll open a PR later to add it, then we can consider deprecating ValueConverter::fromPhp. Wdyt?

@stof
Copy link
Member

stof commented Jan 13, 2023

Well, actually, support converted values in that one is something I want to keep, because it will make it simpler when releasing 2.0 that will introduce the modern representation of values. When passed with a legacy value (the current ones), it will convert it to modern ones. So we cannot deprecate it.

@uuf6429
Copy link
Author

uuf6429 commented Jan 13, 2023

I see... I'll give the v2 a look then, if it's available.

@stof
Copy link
Member

stof commented Jan 13, 2023

not yet

@stof stof added this to the 2.0 milestone Sep 14, 2023
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

No branches or pull requests

2 participants