Skip to content
This repository was archived by the owner on Jan 5, 2018. It is now read-only.

Conversation

@yuvraj03
Copy link

Finishes the annotation plugin by adding Category and making corresponding changes to EmbedTypeInterface and EmbedTypeManger.

Copy link
Member

Choose a reason for hiding this comment

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

this usage of array_push is wrong

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this still needs to be addressed?

Copy link
Member

Choose a reason for hiding this comment

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

This variable is not defined.

Copy link
Member

Choose a reason for hiding this comment

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

The $definitions_for_category variable is still undefined at this point. It needs to be initialized to an empty array before the for loop. Also, we should just be using a foreach ($defintions as $definition) instead of a for loop.

@davereid
Copy link
Member

davereid commented Jul 1, 2015

I'm actually not sure why we need a category for the embed annotation at all? What kind of values would example implementations store here that wouldn't duplicate the ID itself? For example, if I created an Entity Embed display plugin, I'd probably be adding an embedType = "EntityEmbed" which would match the @EntityEmbed embed type plugin ID. As such, we could probably remove getDefinitionForCategory(). Thoughts @cs-shadow?

@cs-shadow
Copy link
Member

@davereid the idea here was that more than one modules can provide mehtods to embed the same kind of thing. So, if someone wants a different implementation of entity_embed, he can do so by keeping the category same and the ID different and finally one can somehow "choose" what module one wants to use for any type of thing if there are more than one.

So, this was just to add bit more flexibility but we can do away with it if you wish so.

@davereid
Copy link
Member

davereid commented Jul 1, 2015

Hrm, I think they would alter the Entity Embed annotation definition instead, instead of providing an alternate plugin. I think right now I don't see a use case for category, so let's just remove it.

@cs-shadow
Copy link
Member

Okay, we can go with that.

Now, if we want to get definitions for a category from the plugin manager
then what should we call that function since we won't call it
getDefinitionsForCategory as we don't have category anymore.

On Thu, Jul 2, 2015 at 12:25 AM, Dave Reid notifications@github.com wrote:

Hrm, I think they would alter the Entity Embed annotation definition
instead, instead of providing an alternate plugin. I think right now I
don't see a use case for category, so let's just remove it.


Reply to this email directly or view it on GitHub
#3 (comment).

Regrads,
Chandan Singh
http://chandansingh.net/

@davereid
Copy link
Member

davereid commented Jul 1, 2015

I don't think we need to care about categories? We'd just want to use $pluginManager->getDefinitions() instead?

@cs-shadow
Copy link
Member

Gotcha!

I initially thought we wanted a function where we could pass 'entity' as
argument and it would return the definition of 'entity' type plugin.

On Thu, Jul 2, 2015 at 12:35 AM, Dave Reid notifications@github.com wrote:

I don't think we need to care about categories? We'd just want to use
$pluginManager->getDefinitions() instead?


Reply to this email directly or view it on GitHub
#3 (comment).

Regrads,
Chandan Singh
http://chandansingh.net/

@davereid
Copy link
Member

davereid commented Jul 1, 2015

The only use case I can think of for listing out these plugins is on the Embed button form for "what type of thing do you want to embed?" which I think using the plugin id/label should be preferred.

@cs-shadow
Copy link
Member

Okay, sounds good.

Lets go with that.

On Thu, Jul 2, 2015 at 12:38 AM, Dave Reid notifications@github.com wrote:

The only use case I can think of for listing out these plugins is on the
Embed button form for "what type of thing do you want to embed?" which I
think using the plugin id/label should be preferred.


Reply to this email directly or view it on GitHub
#3 (comment).

Regrads,
Chandan Singh
http://chandansingh.net/

@davereid
Copy link
Member

davereid commented Jul 1, 2015

I'm a little confused about the progress of this. We already have an annotation class in src/Annotation/EmbedType.php. It looks like we just need to fix EmbedTypeInterface to extend PluginInspectionInterface. There is no need for all these additional methods on the interface.

@cs-shadow
Copy link
Member

@davereid That was my bad. I added those and forgot about PluginInspectionInterface.

Is there even a point of this PR if we don't want to add category to this?

@davereid davereid closed this Jul 2, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants