[Meta] Code cleanup migrating to Drupal 8 version

Hi Kalin,

thanks for pushing your code. I was just reviewing it (http://cgit.drupalcode.org/taxonomy_manager/commit/?h=improvement/262440...).

There is one "code style" I would not change, although I know PHPStorm does that automatically for you:

-          TaxonomyManagerTree:self::getFirstPath($term_to_expand, $list);
+          TaxonomyManagerTree:
+          self::getFirstPath($term_to_expand, $list);

or

-        $term = \Drupal::entityTypeManager()->getStorage('taxonomy_term')->load($tid);
+        $term = \Drupal::entityTypeManager()
+          ->getStorage('taxonomy_term')
+          ->load($tid);

So in your version every method call gets a separate line. I think it makes the code longer (more lines), and as long as the single-line-statement is not far too long, also more difficult to read. Furthermore core uses the single-line statement too, as far as I can see.

Apart from that, everything looks good :) Thanks a lot! And don't wait to merge it into the main branch, otherwise you might have to fix conflicts with my commits in the meanwhile ;)

Apart from the code style itself, I found some other areas we should address

• Class names: I think we do not have to prefix everything with TaxonomyManager, as we have our own namespace. Before we change anything in code we can discuss class names here.
• Paths in routes: currently it's taxonomy_manager/..., probably taxonomy-manager/... is cleaner. (from underscore to dash)
• Directory structure: I use js/custom/tree.js and js/custom/termData.js. As there is no other code anymore (like the fancytree library), probably it's better to remove the 'custom' folder and move the files just to 'js/'
• Complete code documentation in general (methods, parameters, etc.)
• Verify code via code sniffer: http://pareview.sh/

Thanks for pushing your changes, they look good :)

Regarding the class naming, before we change anything I will do some more research too, see if there is some documentation on class naming conventions and ask others. My ideas are similar to yours and I would also add the type (Controller / Element / Form / ...) at the end of the class name, like TreeElement (so you immediately know this class is about a form element)

For the rest, feel free to directly push your changes to the main branch.

@Directory structure - yes, and remove old/ in addition?
-> Yes, remove the old stuff. Then it's also easier to use http://pareview.sh/

Regarding the class naming, this is the documentation I was able to find:
https://www.drupal.org/node/2156625
https://www.drupal.org/node/1353118

Thanks for the updates!

Regarding the class naming, also talked to klausi, so let's keep them short (without TaxonomyManager in general, as we have the namespace), but still it should be clear what it does, so just "Main" might be too generic.

My thoughts on naming:
src

• Controller
  • VocabularyListController (currently MainController, but IMO too general)
  • SubTreeController
• Element
  • TreeElement
• Form
  • SettingsForm (currently TaxonomyManagerAdmin)
  • TaxonomyManagerForm / ManagerForm / VocabularyForm / ... ? (not sure for this one)
  • AddTermsForm
  • DeleteTermsForm
  • MoveTermsForm
  • ExportTermsForm
• TaxonomyManagerHelper (lets leave the name for this one for the moment)

What do you think?

VocabularyListController -> it's ok, I would be ok as well with VocabularyController

Maybe VocabularyOverviewController?

Otherwise, TaxonomyManagerForm as it is now sounds ok for me as it's eventually the essence of the project and "deserves" to be so concrete? :)

Agreed :)

I opened a separate ticket for array format cleanup:

#2959667: Convert use of array() to short form.

As mentioned in #2960831: Migrate from drupal_set_message(), this issue is quite broad in scope. Since all patches have been committed in THIS issue, I feel converting into a Meta issue would increase clarity and help get the cleanup moving in a more orderly manner.