Hello Mark,

Thank you for all your hardwork in giving the drupal community a module making our sites a bit more stylish.

I was wondering what the status of the project is while heading into drupal 8?

I believe https://www.drupal.org/project/drupalmoduleupgrader is useful for this.

Although I'm not a developer, I'd gladly test and provide useful feedback for the drupal 8 module.

Kind regards

Phil

Comments

inders’s picture

StatusFileSize
new151.32 KB

- Attaching my Initial code for Drupal 8 version of icon module.

What is done so far in this?

  • Used drupalmoduleupgrader module for upgrading to 8.x version
  • Fixed couple of errors to make it install-able.
  • Added few yml files as were needed.
  • Bundle overview form is accessible at: admin/config/media/icon . Although not able to save all configs.

Thank you !

markhalliwell’s picture

Status: Active » Needs work
inders’s picture

Status: Needs work » Needs review
StatusFileSize
new138.11 KB

- Attached patch at here
Please review.
Thank you !

Status: Needs review » Needs work

The last submitted patch, 3: icon-d8upgrade-2529032-1-8.x-1.x-dev.patch, failed testing.

The last submitted patch, 3: icon-d8upgrade-2529032-1-8.x-1.x-dev.patch, failed testing.

The last submitted patch, 3: icon-d8upgrade-2529032-1-8.x-1.x-dev.patch, failed testing.

The last submitted patch, 3: icon-d8upgrade-2529032-1-8.x-1.x-dev.patch, failed testing.

The last submitted patch, 3: icon-d8upgrade-2529032-1-8.x-1.x-dev.patch, failed testing.

inders’s picture

StatusFileSize
new140.28 KB

Updated patch at here.

markhalliwell’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
StatusFileSize
new13.82 KB

Thanks! Since this patch isn't yet complete, I'd much rather keep it in here until it's remotely usable.

Also, I've taken out d.o's packaging modifications (LICENSE.txt and sub-module.info files) and updated the 8.x-1.x branch to reflect recent 7.x-1.x code.

Moving forward, let's rebase off of the 8.x-1.x branch.

inders’s picture

StatusFileSize
new41.53 KB

Few more changes added in patch:-

  1. Used module upgrader for submodules: icon_block, icon_field, icon_filter, icon_menu
  2. Fixed errors which were detected after applying previous patch on 8.x-1.x branch.

#What is functioning so far?

  1. Module enable & disable.
  2. Icon configuration page
  3. Icon bundle enable/disable

#What is next step?

  1. Theme Icon bundle page.
  2. Make Icon Block module work
markhalliwell’s picture

Thanks @inders! I've gone ahead and done a proper review of the primary module (not the sub-modules yet). Here are some notes:

  1. +++ b/icon.links.menu.yml
    @@ -0,0 +1,5 @@
    +book.admin:
    

    Shouldn't this be icon.admin?

  2. +++ b/icon.links.menu.yml
    @@ -0,0 +1,5 @@
    +  route_name: system.icon
    
    +++ b/icon.routing.yml
    @@ -0,0 +1,7 @@
    +system.icon:
    

    This shouldn't start with system (as that indicates it's a system path/module, which it's not). I think icon.overview is more appropriate here.

  3. +++ b/icon.module
    @@ -106,7 +109,7 @@ function icon_menu() {
    -  $items[ICON_ADMIN_PATH . '/bundle/%icon_bundle/icons'] = array(
    +  $items[ICON_ADMIN_PATH . '/bundle/%con_bundle/icons'] = array(
    

    Somehow the i got removed right after %.

  4. +++ b/icon.module
    @@ -274,7 +277,7 @@ function &icon_render_hooks($hook = NULL, $reset = FALSE) {
    -  $bundle += array(
    +  $bundle_extra = array(
    
    @@ -284,6 +287,7 @@ function icon_bundle_defaults(&$bundle = array(), $name = '') {
    +  $bundle = array_merge($bundle_extra, $bundle);
    

    The += operator should be sufficient here as its only purpose is to add in defaults if none are provided. These changes aren't needed.

  5. +++ b/icon.module
    @@ -303,7 +307,7 @@ function icon_bundle_defaults(&$bundle = array(), $name = '') {
    +    if (!$reset && ($cache = \Drupal::cache()->get('icon_render_hooks')) && !empty($cache->data)) {      ¶
    
    @@ -578,7 +587,7 @@ function icon_provider_defaults(&$provider = array(), $name = '') {
    +    if (!$reset && ($cache = \Drupal::cache()->get('icon_providers')) && !empty($cache->data)) {      ¶
    
    +++ b/includes/cache.inc
    @@ -17,7 +31,8 @@ function icon_flush_caches() {
    -  cache_clear_all('icon_', 'cache', TRUE);
    +  \Drupal::cache()->deleteAll();
    +  ¶
    

    Whitespace at the end.

  6. +++ b/icon.module
    @@ -338,7 +342,7 @@ function icon_bundles($name = NULL, $reset = FALSE) {
    -        if ($bundle === FALSE) {
    +        if (!empty($bundle) && $bundle === FALSE) {
    

    This would never reach the second conditional and is, quite frankly, a separate issue if there's a bug here (i.e. has nothing to do with porting).

  7. +++ b/icon.module
    @@ -429,11 +434,11 @@ function icon_bundle_delete(array $bundle = array()) {
    -    drupal_set_message(t('The icon bundle %bundle has been disabled.', array('%bundle' => $bundle['title'])));
    +    drupal_set_message(t('The icon bundle %bundle has been disabled.', array('%bundle' => $bundle['name'])));
    ...
    -  drupal_set_message(t('An error occurred while attemping to disable the icon bundle: %bundle.', array('%bundle' => $bundle['title'])), 'error');
    +  drupal_set_message(t('An error occurred while attemping to disable the icon bundle: %bundle.', array('%bundle' => $bundle['name'])), 'error');
    
    @@ -449,11 +454,11 @@ function icon_bundle_disable(array $bundle = array()) {
    -    drupal_set_message(t('The icon bundle %bundle has been enabled.', array('%bundle' => $bundle['title'])));
    +    drupal_set_message(t('The icon bundle %bundle has been enabled.', array('%bundle' => $bundle['name'])));
    ...
    -  drupal_set_message(t('An error occurred while attemping to enable the icon bundle: %bundle.', array('%bundle' => $bundle['title'])), 'error');
    +  drupal_set_message(t('An error occurred while attemping to enable the icon bundle: %bundle.', array('%bundle' => $bundle['name'])), 'error');
    

    Unnecessary changes (i.e. has nothing to do with porting).

  8. +++ b/icon.module
    @@ -536,10 +541,14 @@ function icon_bundle_save(array $bundle = array()) {
    -    if ($status = drupal_write_record('icon_bundle', $record, $primary_keys)) {
    -      icon_clear_all_caches();
    -      return $status;
    -    }
    +     $status = \Drupal::database()->merge('icon_bundle')
    +       ->key(array('name' => $record['name'],))
    +       ->fields(array('bundle' => $record['bundle'], 'status' => $record['status']))
    +       ->execute();
    +     if($status) {
    +       icon_clear_all_caches();
    +       return $status;
    +     }
    

    Improperly indented.

    Also, it seems that this is doing much more than what it's replacing. Needs explanation/commenting on what's going on here.

  9. +++ b/icon.routing.yml
    @@ -0,0 +1,7 @@
    \ No newline at end of file
    

    Missing new line at EOF.

  10. +++ b/includes/cache.inc
    @@ -17,7 +31,8 @@ function icon_flush_caches() {
    -  cache_clear_all('icon_', 'cache', TRUE);
    +  \Drupal::cache()->deleteAll();
    

    This is now deleting all cache. It should only be deleting the icon_ prefixed cache bins.

  11. +++ b/includes/utilities.inc
    @@ -359,7 +362,18 @@ function icon_find_theme_include($theme) {
    -      $includes = array_keys(drupal_system_listing('/icon.inc|icons.inc$/', 'themes', 'uri', 0));
    +      $includes = array();
    +      $listing = new ExtensionDiscovery();
    +      $available_themes = $listing->scan('themes');
    +      foreach ($listing->scan('themes') as $theme) {
    +        if (file_exists($filepath = $theme->getPath() . '/icons.inc')) {
    +          $includes[$theme->getName()] = $filepath;
    +        }
    +        else if(file_exists($filepath = $theme->getPath() . '/icon.inc')) {
    +          $includes[$theme->getName()] = $filepath;
    +        }
    +      }
    

    This entire code snippet doesn't quite look right. I've read https://www.drupal.org/node/2198695, but even that seems a bit cryptic. There's got to be an easier way to do something similar like the "preg" one liner before.

hatuhay’s picture

Hello both,
Got a quick look over the code and my perception is that this module code is going somehow not on the best path for the discovery and handle of bundles.
Drupal 8 way, encourage the use of YAML files, so the modules should declare the icons in something like:
MY_MODULE.icons.yml:

lullacons_pack1:
  render: image
  provider: lullabot
  title: Lullacons: Pack 1
  icons:
    alert: alert
    arrow-double-down-green: arrow-double-down-green
    arrow-double-down-red: arrow-double-down-red
...

Bundles discovered should be declared as class and apply specific methods: ->get_icons()...

On the other hand, an Icon class should be declared as a repository of icons and should provide icon information and classes for rendering purposes.

Happy to help with the code, but still porting my own modules.

For a quick example, please take a look to Styles API Module. Result is absolutely different but the methods quiet similar.

bbuchert’s picture

Hi,

is this still being ported?

sylus’s picture

StatusFileSize
new43.79 KB
new13.31 KB

I am just attaching a new patch that fixed a few more issues that I found and addresses most of what @markcarver point out except for #11. Attaching an interdiff as well.

sylus’s picture

StatusFileSize
new43.8 KB

Whoops one minor fix:

-    if (!$reset && ($cache = \Drupal::cache()->get('icon_render_hooks')) && !empty($cache->data)) {      
+    if (!$reset && ($cache = \Drupal::cache()->get('icon_bundles')) && !empty($cache->data)) {

Should still be icon_render_hooks.

sylus’s picture

StatusFileSize
new43.63 KB

Sorry for noise spotted one other incorrect placement.

nikathone’s picture

Status: Needs work » Needs review

How about the maintainer of this module commit the latest patch then create a meta issue which will be listing outstanding work?

  • markcarver committed e9b19b3 on 8.x-1.x authored by inders
    Issue #2529032 by sylus, inders, markcarver: Upgrade To Drupal 8
    
markhalliwell’s picture

Category: Feature request » Task
Status: Needs review » Needs work

Ok. I've committed the latest patch, just to keep this issue going (this is the "meta" btw).

However this issue/project still needs a LOT of work done.

Specifically, I think it would make more sense to turn all the provider/bundle info definitions into plugins.

There's also a lot of "@todos" and logical inconsistencies in the above code.

sjpeters79’s picture

StatusFileSize
new12.31 KB

I've started working on migrating submodule icon_field over to d8.

sjpeters79’s picture

StatusFileSize
new0 bytes

I've updated the previous patch with a working version of the icon_field module. Only thing missing at the moment is the feeds implementation.

sjpeters79’s picture

StatusFileSize
new19.08 KB

Sorry uploaded the wrong file in last comment.

sylus’s picture

We have had a 8.x release out for 4 months but due to all projects now being open; of course there is now another one:

https://www.drupal.org/project/icons

What is the plan for this, have tons of duplicates or try to force this as official. I knew this would start to happen once they opened the gates.

With the old workflow at least everyone was encouraged to check for existing ones and improve them. I would have loved if the work gone into icons went into icon and kept the developers together in a single project.

Christopher Riley’s picture

The problem I see is that there is a push to get on the 8.x bandwagon and this module or one of similar functionality is a must for those that are building sites that do not have the ability or the understanding to write the code needed to do such customization.

I just grabbed the latest code and applied that patch but you still cannot activate menu icon because it is still looking for the menu module which doesn't exist any longer, menu block is still looking for old tables etc.

So what is the average Drupal site owner supposed to do other than look at the alternate modules that are being maintained and hope that they find something that is close until the project they want to use becomes a priority.

Just my two cents.

sylus’s picture

Yes that makes sense, the problem is now that we have different developers in different projects that are supposed to do the same thing. So which project should we be working on?

drupal.org/project/icon

is the longer maintained one which is where most of the users are tracking against. The main module has been ported as well as the sub module for fields. We still need to convert the sub module for menu and implement as a Plugin API for the bundles.

drupal.org/project/icons

Focused on getting the menu link stuff working and already includes leveraging the Plugin API but doesn't leverage sub modules for the extra integrations.

Ideally we port the work done in drupal.org/project/icons back to this project, but honestly not sure ^_^

Christopher Riley’s picture

In the end I really think it is up to Mark or one of the project maintainers to decide. I myself would prefer to see this project get up to snuff since those that use the module in 7.x will surely want to stay with something that they know works and is stable.

markhalliwell’s picture

... the problem is now that we have different developers in different projects that are supposed to do the same thing.

People "fork" stuff all the time. Unfortunately, that's just going to be the reality now. Instead of people contributing to existing projects, they'll create their own. This is usually because they don't want to follow an existing pattern. This is evident by this new project since they bundled "menu items" with the main project.

The "reason" I made icon_menu a separate submodule (as with the others) is that not everyone will want or need certain functionality.

By forcing a site to always have said functionality enabled, it actually prevents whomever is in charge of building/customizing the site from placing constrains in areas that people shouldn't have the ability to customize (e.g. you want end-users to have access to icons in blocks, but not menus because the theme constructs the site menus and uses the main module's API via render arrays).

This is why the main module is just an API. The sub-modules utilize said API to provide specific UI implementations of that API for easier implementation.

There are sites where I use just the API, not sub-module enhancements because they're confusing to the end-users and/or they use them inappropriately.

---

In fact, this is their primary "claim of difference":

This module offers menu support out of the box because it is one of the main "sitebuilding" features where someone wants to use icons in general.

While technically, this may be true (from an "out-of-the-box" standpoint), however the reality is that it doesn't actually offer anything "new" or truly "different".

Furthermore, they provide icomoon and fontello sub-modules bundled with this project. Which, again is duplicating existing efforts.

From my perspective, someone basically took this the concept (and probably some code) of the existing projects and decided to just implement their own.

Is what they did disrespectful? Yes.

Does it continue to fragment our community? Yes.

Could they, instead of creating their own project, contributed to this one? Yes.

Did they even attempt to contribute to this project (even something as simple as creating issues)? No.

So which project should we be working on?

This one.

I myself would prefer to see this project get up to snuff since those that use the module in 7.x will surely want to stay with something that they know works and is stable.

So would I. Unfortunately, it would seem that the burden continues to fall on my shoulders as the module maintainer to truly move this project in the right direction.

I would love to see more patches like above, but that requires others to actually do the work (which I myself haven't had a lot of time for lately).

I'll commit the above patch shortly after this comment.

  • markcarver committed 38eec3a on 8.x-1.x authored by sjpeters79
    Issue #2529032 by sylus, inders, sjpeters79, markcarver: Upgrade To...
markhalliwell’s picture

We'll see how this goes....

Christopher Riley’s picture

Thank you Mark we do appreciate your efforts!

Christopher Riley’s picture

Thank you Mark we do appreciate your efforts!

jefuri’s picture

Hey markcarver,

Got your issue and get your point while reading back the discussion about a determining which drupal icons project should be the leading one to be used.

To be honest, I do not really care which one it is.

What I do care about is that is one that should work. When I started to work on my version of an icon module, this module was not that far in it's d8 development and looking at the d7 version I did not quite agree with the setup. Which is irrelevant to the question we have in this issue, I agree.

If you want to merge my code back to this project and continue here, I'm totally fine with it. The only reason is that we needed (where I work) a total usable icons module (in our perspective).

I still feel that the version in the d8 branche of this project is not quite usable yet.

But please do not start to rant about disrespect and opening the gates with the new project application and flow or other stuff. I worked on this for the last 2 months and also put a lot of my spare time in this. In this case, it is you who shows little respect for the effort I have put it in.

And yes you are right, I could have tried to put in an issue here first or try to contribute to this project. But since development here was a bit low and some concepts in this module are different from what I have in mind for the d8 version, I decided to start over. I apologize if I offended anyone with this decision.

But enough bashing, let's start to be more constructive. I agree with you all on the following points.
- We should have one single Icons module voor d8.
- Menu link integration should be a sub module
- icomoon and fontello integration should be ported to the similar named projects
Disagreements:
- icon_field, to me it could be in the core module. But also supplying a widget to select icons with a nice visualization
- My version does not only attend the menu/site building issues but also makes it complete possible to render icons with a very simple render array. Which is something I could not quite find in the source code of 7.x and 8.x in this module.

Anyway, what would be the best way to go about this? Because my module focuses more on the use of configuration entities as a base for available icon sets within a drupal installation.

markhalliwell’s picture

If you want to merge my code back to this project and continue here, I'm totally fine with it.

I do, but I doubt I'll have time to do it myself right now anyway as I have client work.

But please do not start to rant about disrespect and opening the gates with the new project application and flow or other stuff.

It's not a rant, it's just a fact. That project is basically an exact duplication of existing efforts (on all fronts), it just goes about it in just a very slightly different way.

I worked on this for the last 2 months and also put a lot of my spare time in this.

Just how long do you think I've spent on this and other projects? I took over this project's namespace 4 years ago (to the day and longer than you've had your own account, BTW) and also spent countless hours of "spare time" to get this and other projects up and going.

In this case, it is you who shows little respect for the effort I have put it in.

Do you understand how ludicrous this statement is? All this effort could have easily be put into existing projects to benefit the multitude of people who already use them. Furthermore, it's not like some of this effort cannot be merged into this project. I wasn't being disrespectful, just honest.

But since development here was a bit low

It's "low" because it's a relatively stable module (for 7.x). The 8.x port has been "low" because I haven't had time to actively work on it myself.

It doesn't change the fact that I get email notifications for this project and would have happily reviewed, given feedback and committed patches if someone had show active interest/desire in a 8.x port (as you can see by my commits from above).

The only reason the above patches sat for 7-11 days is because I was in still in the middle of client work. It doesn't change the fact that this project is still in my own personal "queue" of things to get to.

The point is: you didn't even try. It's not like I would have said "no, don't help me port this module" lol

some concepts in this module are different from what I have in mind for the d8 version

The only two "concepts" that are truly different are:

  1. Enabling functionality in main module vs sub-modules for the UI functionality aspects.

    As I've already explained above, this is by design since the main module is only an API and does not provide any UI components.

    The sub-modules are intended to be enabled as needed (each with their own separate permissions) to enable functionality only to the desired users/roles.

  2. Icon "sets" as config entities vs. icon "bundles" as static resources.

    Icon bundles don't need to be entities and is, honestly, complete overkill. Once icons are there they rarely change and, when they do, can easily be replaced and then their definitions rebuilt.

    They are not, usually, provided by individual site users and have no need to muddy the content driven areas. In 7.x these bundles are defined as info definition hooks and in 8.x., they should likely be turned into discoverable annotated plugins.

icon_field, to me it could be in the core module. But also supplying a widget to select icons with a nice visualization

Again, forcing functionality that isn't always desired. In large sites, where there are 10s and sometimes hundreds of users, opening up the ability to "add" something needs to be granular. It isn't an accident that these sub-modules were designed the way they are.

My version does not only attend the menu/site building issues but also makes it complete possible to render icons with a very simple render array. Which is something I could not quite find in the source code of 7.x and 8.x in this module.

The "source code" is the theme hook the API defines.

Also, it's on the project page...

  $element[] = array(
    '#theme' => 'icon',
    '#bundle' => 'my_bundle',
    '#icon' => 'my_icon',
  );
  print drupal_render($element);

---

Anyway, what would be the best way to go about this?

Deprecate the duplicate project and focus efforts here and the subsequent related modules.

I'm really not trying to be unreasonable.

I'd even be willing to make you a co-maintainer of all the various projects once I'm comfortable that the provided patches follow the architecture/workflow of the existing modules.

The reality is: I definitely need help because I cannot do it all on my own. I maintain a ton of projects.

markhalliwell’s picture

@sylus, would you be willing to become a co-maintainer too?

jefuri’s picture

But because I feel judged (not on a positive way) I do want to say one last thing. Do not judge someone just because someone might have made a bad or less well thought out decision. It is not a sign of disrespect, it's a mistake I made at that moment.

Yes and no to some of your comments. But what I said, enough pointing out our opinions on wether or not I was right or not to do this. Because as I admitted, I could have shown more effort by trying to contact and contribute to this project. If that would have stalled or wasn't a productive way to get a working d8 version on here I always could have done this afterwards. But could have taken much more time to get a module in d8 that could be used.

As you might have realised, some processes on drupal are not that streamlined and we kind of needed a working version voor a d8 project that could be required with the composer workflow.

I do appreciate the fact that you care and you want this to be standardized and offer me the chance to contribute my efforts back to this project.

Conclusion: Let's stop arguing about this, but let's start discussing about the architecutre of the d8 module.

The reason I chose to use config entities is to make it more releasable with features/modules/themes and such. Which in retroperspective might be to large for it's purpose. Using caching for specific icon set/bundle type information would be a better solution (looking at this project). Maybe the right way is somewheren in the middle?
I haven't checked it out yet but isn't it possible to release configuration with a theme for instance? I also could imagine that some icon sets are somewhat theme specific. Or how do we see bundle configuration within d8 with the d7 setup when releasing?

Anyways, I got some todo's for this week now. Starting today I will prepare a rewrite of my module into a d8 version of icon while trying to stay true to d7.

These are on my todo:
icon
- icon_field
- icon_menu

First we need some new issues to separate these tasks in and discuss the development of each (sub)module.

icon
- using plugin api
- caching for available icons
- offer each plugin the choice for theming
- take release management into account (configuration management)
- ability to enable/disable icon bundles

icon_field
- field schema setup
- field widget setup (nice select dropdown with icon examples)
- port (some) configuration options from d7 icon module (like the prefixing/suffixing)

icon_menu
- icon field would pocess some generic way to configure icon and it's output, would it be a dependency
- port (some) configuration options from d7 icon module (like the prefixing/suffixing)

sylus’s picture

Thought I would just chime in!

First off a big thank you to both @markcarver + @jefuri. Really happy we have a path forward to this.

Additionally apologies on my behalf if I went full tilt on projects being open. It was the first time I saw the page since the change and seeing another similar metatag module and few other similar projects triggered me. That being said @markcarver is completely right that this is the new way of things and to be expected when functionality needs to differ.

Me + sjaffery who submitted earlier patches actually work together so this all started because we weren't quite sure whether to keep contributing patches to this project or not. Given the above discussion this is all sorted out now and again my huge thanks to you both!

@sylus, would you be willing to become a co-maintainer too?

I'd happily help by becoming a co-maintainer! Something none of us wants to happen is to have @markcarver burned out. It probably isn't said enough but definitely appreciate the great work you do especially with modules such as bootstrap + bootstrap_layouts :)

Additionally I will happily help review / move over any logic from the other icon project to minimize hassle for @jefuri.

sylus’s picture

Ah just saw @jefuri's new post above and I like the breakdown, thanks so much for taking the time!

Only thing I noticed is I think the icon_field port is completed? Next one to attempt that makes sense might be icon_menu? ^_^

jefuri’s picture

It ain't a hassle to help out. I'm quite proud on the stuff I build till now in the other module, probably the reason I kind of got "too" pissed about the stuff I saw in this issue. So I would like to help out porting it to this project. And if @markcarver still wants me as a maintainer, I would gladly help out. Also on the other projects that integrate with the icon module.

@Sylus, there is some stuff from my module that can be ported for the widget. Creating a styled dropdown with an example of the icon. Currently (in the patch) is see a textfield instead of a dropdown which does not seem user friendly right?

Do think this issue is getting bloated. Do we agree that we should create separate issues to target individuel parts of the porting?

One for the core module port.
One for each submodule.

Another suggestion to offer a somewhat better and faster flow of development. Maybe temporary move to github so we can all clone and develop and merge back through pull requests. I saw that a lot of big projects use this workflow here. Drupal commerce being one of them.

The main icon project on github could be a clone from drupal that periodically merges code back to drupal git.

markhalliwell’s picture

But because I feel judged (not on a positive way)

For this I must apologize. My intention was never to judge a single individual. My frustration was only with the duplication of efforts. It happens a lot more than you may realize and often feels like the first blow, intentional or not.

Let's stop arguing about this, but let's start discussing about the architecutre of the d8 module.

Agreed.

The reason I chose to use config entities is to make it more releasable with features/modules/themes and such.

Config is a standalone API and not directly tied to entities. We can achieve the same goal with annotated plugins + module specific config (for custom/user uploaded bundles, like icomoon, fontello, etc.).

I haven't checked it out yet but isn't it possible to release configuration with a theme for instance?

Technically, yes.. but config in themes is really only meant for initial install (there's a few issues/confusion going around this topic).

Instead, I would imagine that'd we'd actually create discoverable plugins (via PHP annotations + yaml as @hatuhay suggested in #13).

This is actually what is being done in https://www.drupal.org/project/bootstrap_layouts (so themes can provide specific layouts).

The annotated plugin API was, historically, only intended for modules. However, it can easily be adapted to work with themes too. See: BootstrapLayoutsPluginManager.

The plugin API also already has alterability and cacheability built in as well, so we don't have to worry about that (too much).

---

In 7.x is that a theme can provide a icons.inc file that provides the necessary hooks. In the case of Bootstrap, this is done in both bootstrap_icon_bundles() and bootstrap_icon_providers().

In 8.x, we should obviously convert procedural hooks into plugin definitions instead.

---

I'd happily help by becoming a co-maintainer!

Cool. @sylus, I added you to Icon API, Icomoon, Fontello and Bootstrap.

sylus’s picture

Hi @jefuri yeah your right lets start breaking out issues and just adding a relationship back to this one.

Also I completely agree you have done some great work over in the other module. It is obvious that you have spend quite some time on it. Any improvements that you think would be beneficial please don't hesitate to bring up here. :)

markhalliwell’s picture

And if @markcarver still wants me as a maintainer, I would gladly help out.

I do. Once I'm comfortable with your understanding of this how this API works and my vision for it, I'll gladly add you to help co-maintain it.

It'll probably be once we get a stable release and we're all happy with the end result (so we all know how it should work).

Until then, I've added @sylus to help out when I'm not around because he's very familiar with my projects, Drupal coding standards and the overall vision of how to modularize and abstract code.

Creating a styled dropdown with an example of the icon. Currently (in the patch) is see a textfield instead of a dropdown which does not seem user friendly right?

This is likely breakage from the initial port. The 7.x version has a icon_selector element type that probably needs to be converted.

I'd recommend getting yourself familiar with the 7.x version first more than anything.

This will help you understand the scope of this project and what functionality is missing/broken for the 8.x issues.

I'd also recommend going through the existing issue queue as there are probably a lot of issues that already exist that can be tackled/repurposed for 8.x and then (maybe) backported to 7.x.

An example of one (in relations to the icon selector) is #2370899: Show icon on icon_selector dropdown list.

It'll also help give you some history about a lot of this project.

Do think this issue is getting bloated. Do we agree that we should create separate issues to target individuel parts of the porting?

Yes, let's convert this to just a plan and add this issue as a parent to all the child issues that are created.

Maybe temporary move to github so we can all clone and develop and merge back through pull requests.

I'd rather keep development on d.o and use patches. I use specific tools/workflows that are conducive to my maintainership of d.o projects. You shouldn't have any major blockers since both I and @sylus can commit your patches.

markhalliwell’s picture

Category: Task » Plan

Hm, it didn't switch to a plan... trying again.

sylus’s picture

Thank you so much @markcarver!

I will use these newly granted permissions wisely and with greatest of care!

@jefuri I'll make sure to follow the upcoming related issues in a timely manner and do everything I can to help you with whatever you may need so can get this out of the door. I'll still probably defer to @markcarver for most high level issues but hopefully we can minimize some of the lower level issues that he would otherwise have to address :)

sjpeters79’s picture

Thanks everyone for working things out and working together to get this done.

Also just to update @jefuri in regards to your todo list:

These are on my todo:
icon
- icon_field
- icon_menu

I've already got icon_field working as part of patch 23 which was merged in so if you download the latest code, you should have that sub-module. Please review it and make any changes you see fit.

jeff veit’s picture

Since the initial run through drupalmoduleupgrader, there have been improvements in both the module upgrader and in the 7.x version of icon - for example wrappers. I've run the latest 7.x through the upgrader and there are quite a lot of changes compared to the current 8.x-dev. Would you be okay with a large patch to incorporate all the changes, or smaller patches that split out the 7.x upgrades? I would prefer delivering a monolithic patch. Probably no surprise.

markhalliwell’s picture

Yes, I was wondering about that myself. I couldn't remember if that was before or after the 8.x stuff started. Please create a new issue to track this though. A large patch should be fine.

jeff veit’s picture

I got a bit further with this, but I think I have to do it piecemeal and merge the results to make sure that I catch the changes made to the current 8.x-dev, in order to make certain they aren't overwritten.

And yep - I'll add another issue.

sjpeters79’s picture

StatusFileSize
new15.76 KB

I've modified icon_field submodule to use select field instead of text field and I've updated the widget and formatters to accomodate this change. Also I've started building the autoload functionality to consume all plugins managed by IconSetPluginManager. Right now the attached patch works for the most part but there's a bug where the default values aren't being accepted and I'll work on that over the next couple of days.

markhalliwell’s picture

@sjpeters79, could you use/create separate issues so we can start tracking them. It would be really helpful.

Also, may I ask that you not bundle any "select picker" CSS/JS with this since it was never part of the original module and we already have a separate issue for this desired functionality and it will need some seriously thought and planning to implement it properly: #2370899: Show icon on icon_selector dropdown list.

I'd really like to only do an "initial feature parity port".

We can tackle bells and whistles later.

sjpeters79’s picture

Sure will do.

mstrelan’s picture

I'd like to help in some way, thought I'd do some testing. The 8.x branch here doesn't seem to be usable. I can create an icon field but it seems to just be a text field and nothing is rendered. There were a few PHP notices too. The latest patch in #49 doesn't apply to the 8.x branch. Is there a summary of what might be somewhat working or is there a plan to get something working?

markhalliwell’s picture

Now that #2863343: Create plugin managers for existing procedural D7 API has been committed (recommend reading to get a general sense of what is trying to be accomplished), the focus should now be on #2867824: Migrate procedural functions into plugin managers.

dqd’s picture

@jefuri please go on with the issue opened by markcarver since you both seem to be on a certain agreement: https://www.drupal.org/node/2862237#comment-12305363 (please change the project page to save users time)

arosboro’s picture

Does this module even work for drupal 8? AFAIK icons works and this module's dev branch does not. If that is not the case, please update this issue with instructions to get started.

k3vin_nl’s picture

( I haven't read the whole discussion, so please excuse me if this was already mentioned earlier. )
My colleague just found this D8 project https://www.drupal.org/project/micon that also basically has the same functionality as Icon API and that upon initial impression seems to work nicely. It even has a nice icon selector built in.
So maybe it would be an idea to join forces?

markhalliwell’s picture

Great, another duplicate module...

I really don't understand why people can't just open issues and fix existing code. It's not like I'm unresponsive. I get email notifications for all projects I maintain. I'm just not able to dedicate personal development time to this right now is all. If y'all do the work and supply patches, I'll review them and ultimately commit them once all the issues have been addressed.

markhalliwell’s picture

Related issues: +#2554199: Bootstrap 4

Also, FWIW, BS4 removed it's bundled glyphicons, so we'll likely need this for icon support for future versions of the Drupal Bootstrap base theme.

janes_p’s picture

Is there a roadmap, and ETA or a dev version available in the near future? I am following this thread for about a year now and would love to use the official API on a D8 site. However without an ETA I would at some point resort to a "less official" solution. Thanks for a clarification.

dqd’s picture

Great, another duplicate module...

Oh dear ... *facepalm*

I really don't understand why people can't just open issues and fix existing code. It's not like I'm unresponsive.

Exactly. I absolutely agree with you. The overlapping module clutter has to be stopped. I remember how long it took to convince the community to use ONE library module to manage dependencies and to reuse libraries for different modules.

But nethertheless, since the license policy of Drupal is clear on that, we can shamelessly import its functionality if users want that. That's how libav has been merged back into FFmpeg finally. Anyone? The micon maintainer himself maybe?

janes_p’s picture

As an engineer, I agree that duplicates shall be avoided.

As a project manager, I need the right solution at the right time.

Looking at this issue, it appears that D8 development of this module is abandoned for more than a year now. IMO it is not fair to block other contributors and their initiatives but not progressing the module. In the interest of the community I suggest to transparently look for co-maintainers or a new maintainer and hand over in a structured way.

markhalliwell’s picture

Looking at this issue, it appears that D8 development of this module is abandoned for more than a year now.

Then maybe you need to "look" at the issue again then.

It's a far cry from being "abandoned" as you claim. This is open source. You cannot "put" the responsibility of coding everything on a single individual.

Also, as I have previously stated, my time has been severely limited. This is largely due to client work, but also other major projects I [co-]maintain.

Just because I haven't "actively" worked on this project doesn't mean that other developers are prevented from creating issues and creating patches of their own.

I'm, in all actuality, very responsive with this project and willing to review/commit patches that actually meet Drupal Coding Standards, "Best Practices™" and the logic/decisions already made for how we want to port this module (as I have stated in this issue and others, that are not closed).

It's just that no one has. Care to be the first instead of just complaining about it?

IMO it is not fair to block other contributors and their initiatives but not progressing the module.

No one is "blocking" other contributors. I was only attempting to redirect a duplicate project's development efforts so that it benefits the existing (much larger) install base and stops further fragmenting our community.

That being said, even after the supposed "merge" that developer just went dark and didn't really "do" anything for their project and certainly not for this one. What can I do? Scold them further? No, that's not right. We all have lives, I get it. I was only ever concerned about the duplication aspect, nothing more.

In the interest of the community I suggest to transparently look for co-maintainers or a new maintainer and hand over in a structured way.

You mean like the last sentence of #34 or #35 (where I did making @sylus as co-maintainer)? Again, I think you need to "look" at this issue again.

Responding to comments and reviewing code in patches (for the most part) is a lot less time consuming than having to do the actual work of developing code from scratch.

I would absolutely love to actually flush this module out for D8. There have been several times (especially in this last project I've been involved with) where this module would have been a perfect solution.

Unfortunately, the client didn't have the hours (money) to actively support the port and we (I) were on a time crunch to deliver. This didn't leave much "me time" to work on it.

---

In conclusion, I respectfully ask you to extinguish your torch and store your pitchfork. If you want this issue to continue (faster), encourage others to participate. Don't just attack [co-]maintainers for something they don't always have control over.

janes_p’s picture

For our project I was looking for an official icon field solution (with increasing priority), and I try to avoid introducing "quick hacks", therefore my suggestions to get things going. For those with similar needs I have found such solution in the most recent font awesome module (8.x-2.x). So there will be no more "pitchforking" from my side ;-)

markhalliwell’s picture

Weekly Meeting
Every Thur 3:30 PM CDT (UTC-05:00)
Hangout: https://tinyurl.com/y9o6dm6r
Google calendar: https://tinyurl.com/ybnvlgen
iCal: https://tinyurl.com/y9sy9oem

Edit: corrected URLs

markhalliwell’s picture

Ok, well no one showed up. Granted it was semi-short notice, but I suppose let's try again next week.

mstrelan’s picture

markhalliwell’s picture

Hm, seems the calendar somehow messed up. I ended up creating a new one and I've updated my comment above with the correct URLs

markhalliwell’s picture

Ok. No one showed up for this week's meeting either...

maliknaik’s picture

StatusFileSize
new617.12 KB

This module has been successfully ported to Drupal 8 as part of the Google Summer of Code project.

More information can be found at this issue: #3075891: Complete porting of Icon API: Final Work Submission (GSoC'19)
Github Repository: https://github.com/maliknaik16/icon_api

Please review the patch.

chiranjeeb2410’s picture

Status: Needs work » Needs review
eit2103’s picture

Any updates on when we will have this module ready to use for D8?

maliknaik’s picture

@eit2103 You can use Icon API: Port from D7 to D8 until the patch is merged.

markhalliwell’s picture

Status: Needs review » Needs work

The "patch" in #69 isn't going to be committed as is.

  1. Contains new features that aren't apart of this project (fontawesome)
  2. It's completely unreviewable (too big, likely because of ^)
manuel garcia’s picture

I find this issue confusing, there is already a 8.x-1.x branch, with 8 commits in it. So it looks like the module was ported already no?

Should we close this issue and instead create a "roadmap" with what needs to happen for 8.x-1.0 release?

solideogloria’s picture

I think many people were using the Icon API for Font Awesome anyway, so if the completely new features are an improvement, you should accept it.

As for reviewing, you can view the commit history. #3075891: Complete porting of Icon API: Final Work Submission (GSoC'19)

eit2103’s picture

Wondering if this is ported to Drupal 8.

solideogloria’s picture

See comment #69

markhalliwell’s picture

See #73.

I think many people were using the Icon API for Font Awesome anyway, so if the completely new features are an improvement, you should accept it.

That's not how the Drupal community operates.

Existing projects already have list of defined features and developers expect a "port" to have all the previous features they were using (provided there isn't a viable alternative). Just because you only use one set of features, doesn't negate the use cases of others.

Completely hi-jacking a project, moving it to a separate repository and continuing to direct people there is apathetic to the goals of the community and this project; never mind the fact that it ignores the existing codebase and git history.

If you wish to contribute to this project, I suggest reading the following:

https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupal

dqd’s picture

I am confused ... from what I understood @jefury agreed to merge forces and I had placed a comment under the issue to track that on the project page he agreed to. Now it seems the project is back to live and the whole discussion is obsolete?

Project page after agreement. <> Project page now.

solideogloria’s picture

See these comments. https://www.drupal.org/project/icons/issues/2862237#comment-14049104

My guess is that Font Awesome and ease of use via UI are important to them.