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

[Package Capsules] Broken getModulePermalinkBase #2470

Closed
Tofandel opened this issue Feb 12, 2024 · 2 comments · Fixed by #2471
Closed

[Package Capsules] Broken getModulePermalinkBase #2470

Tofandel opened this issue Feb 12, 2024 · 2 comments · Fixed by #2471
Labels
impact: high a major issue impact most of users type: bug Something isn't working

Comments

@Tofandel
Copy link
Contributor

Description

Package capsules which have a namespace different than config('twill.namespace') (Usually it's a namespace with the package name) will fail at

protected function getModulePermalinkBase(): string
{
$base = '';
$moduleParts = explode('.', $this->moduleName);
$prev = [];
foreach ($moduleParts as $index => $name) {
if (array_key_last($moduleParts) !== $index) {
$singularName = Str::singular($name);
$modelClass = config('twill.namespace') . '\\Models\\' . Str::studly($singularName);
if (! @class_exists($modelClass)) {
// First try to construct it based on the last.
$modelClass = config('twill.namespace') .
'\\Models\\' .
implode('', array_merge($prev + [99 => Str::studly($singularName)]));
// Last option is to search for a capsule model.
if (! class_exists($modelClass)) {
$modelClass = TwillCapsules::getCapsuleForModel($name)->getModel();
}
}
$model = (new $modelClass())->findOrFail(request()->route()->parameter($singularName));
$hasSlug = Arr::has(class_uses($modelClass), HasSlug::class);
$base .= $name . '/' . ($hasSlug ? $model->slug : $model->id) . '/';
$prev[] = Str::studly($singularName);
} else {
$base .= $name;
}
}
return $base;
}
because $name is not a model class but a simple singular no uppercased word

Steps to reproduce

Create a package with 2 capsules with nesting enabled

Go to the route for the nested capsule

Expected result

No error

Actual result

No capsule found for seminar

Versions

Quick fix would be to uppercase $name before getting the capsule

Better fix would be to build on #2423 and improve the way all the classes are located (and maybe add a cache as well)

Tofandel added a commit to Tofandel/twill that referenced this issue Feb 12, 2024
@ifox
Copy link
Member

ifox commented Feb 12, 2024

Hi @Tofandel,

Thanks for reporting this. Capsules were initially meant to be self-contained and were all housed under the same namespace but with the introduction of packages, it is clear that those improvements are welcome. I'm open to reviewing more changes to make resolution cleaner and more robust.

@Tofandel
Copy link
Contributor Author

I PR'd the quick fix for now, I'll send a PR to generalize and allow to configure all the namespaces resolutions sometime this month when I have more time

@ifox ifox added the type: bug Something isn't working label Feb 12, 2024
@ifox ifox moved this to In progress in Twill roadmap Feb 12, 2024
@ifox ifox added the impact: high a major issue impact most of users label Feb 12, 2024
ifox pushed a commit that referenced this issue Feb 14, 2024
@ifox ifox removed this from Twill roadmap Feb 29, 2024
@ifox ifox moved this to Released in Twill roadmap Feb 29, 2024
AidasK pushed a commit to AidasK/twill that referenced this issue May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: high a major issue impact most of users type: bug Something isn't working
Projects
Status: Released
Development

Successfully merging a pull request may close this issue.

2 participants