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

[1.x] Ensure values returns feature name #107

Merged
merged 1 commit into from
Jun 20, 2024
Merged

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Jun 19, 2024

fixes #106

There is an inconsistency when retrieving a list of specific values with Pennant.

When you have a class-based feature that has a name, e.g, $name = 'my-feature';, the Feature::values function will inconstantly return the name and the feature's classname.

Given the following class...

namespace App\Features;

class MyFeature
{
    public $name = 'my-feature';

    // ...
}

We see the inconsistency in the last call to Feature::values:

Feature::all();

// ✅
// 
// [
//     'my-feature' => true,
// ]

Feature::values(['my-feature']);

// ✅
// 
// [
//     'my-feature' => true,
// ]

Feature::values([Myfeature::class]);

// ❌
// 
// [
//     'App\Features\MyFeature' => true,
// ]

Given the main usecase for the Feature::values function is to pass features to the front end...

<script>
const features = @js(Feature::values(['feature-1', 'feature-2']))
</script>

<div v-if="features['feature-1']>
    <!-- ... -->
</div>

...and the main usecase for the $name is to decouple the database and front end from the back end implementation, this feels like a bug that we should address.

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

Comment on lines +421 to +431
/**
* Retrieve the feature's name.
*
* @param string $feature
* @return string
*/
public function name($feature)
{
return $this->resolveFeature($feature);
}

Copy link
Member Author

@timacdonald timacdonald Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the resolveFeature function is protected, I have introduced a new method for this so we didn't have to introduce a breaking change by changing the visibility.

Also, name is a much nicer public facing method name.

We use this internally, but it can also be useful in user-land code.

Feature::name(FeatureWithoutNameProperty::class);
// App\Features\FeatureWithoutNameProperty
Feature::name(FeatureWithNameProperty::class);
// 'feature-with-name-property'

@taylorotwell taylorotwell merged commit 4a3c33d into laravel:1.x Jun 20, 2024
6 checks passed
@timacdonald timacdonald deleted the name branch June 21, 2024 00:33
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.

Use name for class based features in the resulting array when using the values function with a class name
2 participants