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

[TwigComponent] AnonymousComponentRegistry to share anonymous component in your bundles #1456

Conversation

WebMamba
Copy link
Contributor

@WebMamba WebMamba commented Feb 4, 2024

Q A
Bug fix? no
New feature? yes
Issues Fix
License MIT

Let's get crazy! And let the community share anonymous components through their bundles!
This PR introduces a new AnonymousComponentRegistry interface that lets your share the anonymous component inside your bundle

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Feb 4, 2024
@smnandre
Copy link
Member

smnandre commented Feb 5, 2024

About this PR

I have not much time, but please read attentively what i wrote in the other topic i shared on slack about this topic i've been working on since some months now.

There must be a unique way to call anonmous components, without what the bundle developpers won't be able to nest together components. There can be a shorten/alias/namespace/xxx one.. but there 100% need one "unique full official name" for a component.

From a DX pov, but a security pov too, there cannot be component that automatically replaces userland ones, or other bundle ones.


Personal disgression

Finally guys, to be 100% transparent, i'm starting to be a bit frustrated to think about things, point where the difficulties can be, what need to be solve, debug things... and then all that beeing ignored as if it did not matter...
.... then finding myself doing a lot of the support concerning issues, debugging things, etc...

Obviously i'm overstating things there, but it's been weeks i'm asking about the way we want to distinct userland components and bundle ones, ... and i have multiple pocs ready, depending on what we decide.

And i'm again in the situation i have to take some grumpy role, explaining once again why some thing can/cannot work.

I have probably a lot to improve in the way i express myself (and i'm 100% open to advices on this -- on everything in fact). I'm just starting to feel a bit tired of working in vain, studiying the internal of Twig, of UX packages, thinking the usages, the limitations, anticipating major security/performances/maintenance issues, and sometimes feeling a bit alone doing all that.

@weaverryan
Copy link
Member

Hey @smnandre -

Really sorry you’re feeling that your efforts, info gathering and POCs are being ignored. I think that’s a fair criticism. Personally, due to time constraints, I have a hard time following all the conversations- especially longer, more technical conversations (and you often do deep research that involves full information). My guess is that this is probably true of others. From experience, I know that PRs (even flawed) tend to illicit more conversation than technical docs. For better or worse, we tend to react more to built things.

So, I hope that helps explain a bit at least. Your efforts around here are HUGE and appreciated. In fact, I think you may be sometimes thinking 3 steps ahead of everyone else. You also, I clearly know, do a lot of the unsung work - support, CI tweaks etc - that help make the project work. I’m not sure what workflow is best for discussing these more complex items, but I’m open to ideas. And, again, from experience, PRs are a powerful tool - even to show how an implementation almost works… but has some flaws. And you can always ping me - but you already know that ;)

@weaverryan
Copy link
Member

Re: this PR.

I also feel that users should control the naming of components - not the bundle itself. I’m also not convinced that anonymous components need to be allowed for 3rd party packages.

What I think makes sense is a way to “import” a component from a package (and name it when you do) or import an entire namespace (allowing the bundle to provide names), but with an optional namespace prefix. FWIW, all of this would match how Blade components handle things.

Cheers!

@kbond
Copy link
Member

kbond commented Feb 5, 2024

I’m also not convinced that anonymous components need to be allowed for 3rd party packages.

I'm now thinking the same thing. At least until a time there is a significant performance boost between simple class-based components and anon components.

As it stands, 3rd party bundles have an avenue to auto-register there own class-based components. While not perfect, it works.

@weaverryan
Copy link
Member

I'm thinking that, while bundles can register their own components, we should discourage that (we can't prevent it) and instead offer a nice "import component" option and encourage 3rd party components to be written in a way that makes them NOT automatically registered, but ready to be imported.

Having some YAML config would be the easiest way. But I really hate YAML. So another option, I think, could be a "component provider" system. User creates AppComponentProvider (or even adds an interface to their Kernel and uses that) with a registerComponents(ComponentRegistry $registry) where the can call things like $registry->add('Alert', VendorAlertComponent::class) or an ->addNamespace to register everything from a namespace with default names.

@smnandre
Copy link
Member

smnandre commented Feb 5, 2024

I'd vote for doing things automatically, .... and add a non-configurable bundle prefix.

So vendor/acme/blog-bundle/Twig/Components/SuperWidget.php would be usable as

<twig:AcmeBlog:SuperWidget />

or (to be decided)

<twig:Acme:Blog:SuperWidget />

This is the more "obvious" / "standard" / "safe" way i think ....

If not, i like your provider idea, as long as they use a "prefix", "custom name", "namespace".

The alias idea is something we can (and will) do in a later move, but all this must be working without, so let's focus on that first.
As the alias must only be a userland thing, not something a Bundle controls.

And we want at first give bundle a way to expose components.

@kbond
Copy link
Member

kbond commented Feb 5, 2024

I'd vote for doing things automatically

This'd be my vote as well I think - would be very similar to twig templates in bundles. We would, I believe, still need a way for bundles to let the app know about their components. This could perhaps be the way they also let the app know about their anon components.

@smnandre
Copy link
Member

smnandre commented Feb 5, 2024

In Twig, for templates it's done automatically for any Bundle indeed (registered in the prefixed namespace, and with the ! prefix to be able to reference the original bundle template even when it's overridden in app template/Bundle directories

All is done in two methods, we can do the exact same for both classes and templates

// src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php

// ...

 foreach ($this->getBundleTemplatePaths($container, $config) as $name => $paths) {
    $namespace = $this->normalizeBundleName($name);
    foreach ($paths as $path) {
        $twigFilesystemLoaderDefinition->addMethodCall('addPath', [$path, $namespace]);
    }

    if ($paths) {
        // the last path must be the bundle views directory
        $twigFilesystemLoaderDefinition->addMethodCall('addPath', [$path, '!'.$namespace]);
    }
}


// ... 


private function getBundleTemplatePaths(ContainerBuilder $container, array $config): array
{
    $bundleHierarchy = [];
    foreach ($container->getParameter('kernel.bundles_metadata') as $name => $bundle) {
        $defaultOverrideBundlePath = $container->getParameterBag()->resolveValue($config['default_path']).'/bundles/'.$name;

        if (file_exists($defaultOverrideBundlePath)) {
            $bundleHierarchy[$name][] = $defaultOverrideBundlePath;
        }
        $container->addResource(new FileExistenceResource($defaultOverrideBundlePath));

        if (file_exists($dir = $bundle['path'].'/Resources/views') || file_exists($dir = $bundle['path'].'/templates')) {
            $bundleHierarchy[$name][] = $dir;
        }
        $container->addResource(new FileExistenceResource($dir));
    }

    return $bundleHierarchy;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants