See #1880976: [meta] Port examples to D8

Note that git attribution should include work from #2277667: Port D7 Tests To D8 for menu_example

git commit -m 'Issue #2277667 by mimran, snehi: Port D7 Tests To D8 for menu_example' --author="imran4uall <imran4uall@2712535.no-reply.drupal.org>"
Files: 
CommentFileSizeAuthor
#59 menu_example-1883724-59.patch32.45 KBnavneet0693
#59 interdiff-56-59.txt4.57 KBnavneet0693
#56 menu_example-1883724-56.patch31.37 KBnavneet0693
#56 interdiff-51-56.txt24.06 KBnavneet0693
#51 menu_example-1883724-51.patch24.65 KBnavneet0693
#51 interdiff-46-51.txt3.23 KBnavneet0693
#46 menu_example-1883724-46.patch24.12 KBnavneet0693
#46 interdiff-44-46.txt636 bytesnavneet0693
#44 menu_example-1883724-44.patch24.11 KBnavneet0693
#44 interdiff-40-44.txt9.01 KBnavneet0693
#40 menu_example-1883724-40.patch24.57 KBnavneet0693
#40 interdiff-37-40.txt29.04 KBnavneet0693
#37 menu_example-1883724-37.patch22.77 KBAbhishekLal
#34 menu_example-1883724-29.patch22.79 KBAbhishekLal
#29 menu_example-1883724-28.patch22.79 KBAbhishekLal
#28 menu_example-1883724-27.patch14.32 KBAbhishekLal
#26 menu-examples-1883724-26.patch14.11 KBAbhishekLal
#23 examples-menu-example-test-2277667-28-8.x.patch5.05 KBMile23
#16 menu-example-1883724-16.patch25.86 KBkgoel
#11 2277667_do_not_test.patch4.9 KBMile23
#8 1883724_8.patch5.14 KBMile23
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 371 pass(es), 31 fail(s), and 14 exception(s). View
#2 0001-Issue-1883724-by-marvil07-Start-port-of-menu_example.patch27.42 KBmarvil07
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 342 pass(es). View

Comments

marvil07’s picture

Assigned: Unassigned » marvil07
marvil07’s picture

Assigned: marvil07 » Unassigned
Status: Active » Needs work
FileSize
27.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 342 pass(es). View

Uhm, not yet working. Current problem seems to be that tools menu(the default menu used by D8) is not shown on any region by default, need to investigate about it.

So: Make the tools menu (or define another new menu) and add it to sidebar left region on the theme to show the links and be able to link.

kgoel’s picture

Assigned: Unassigned » kgoel

Going to work on this during DrupalCon lab.

kgoel’s picture

I have worked on it on my sandbox and later in the evening, I will upload the patch for what I have done so far.

kgoel’s picture

Mile23’s picture

Care to offer a patch please?

Mile23’s picture

Mile23’s picture

Issue summary: View changes
FileSize
5.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 371 pass(es), 31 fail(s), and 14 exception(s). View

A straight port of menu_example's tests from 7.x-1.x.

These should fail. :-)

Working on this issue got me working on SimpleTest: #2234295: drupalLogin() crashes where it should fail. #1452896: PHP notice in clickLink if link does not exist

Mile23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: 1883724_8.patch, failed testing.

Mile23’s picture

New failing tests from #2277667: Port D7 Tests To D8 for menu_example. Consider these the 'official' ones, and dev your heart out with them.

mrjmd’s picture

I've got some time in the coming week to try to make some progress on this. I'm going to attempt to contact kgoel directly before assigning this issue to myself. kgoel, if you see this first, have you made any progress beyond what is in the link in #5?

kgoel’s picture

mrjmd- I worked on this during DrupalCon Prgaue so what I have in my sandbox is probably stale anyway.

mrjmd’s picture

Assigned: kgoel » mrjmd

Okay, I am going to assign this to myself for now and will try my hand at an upgrade. This will be my first D8 module upgrade so it's going to be a learning experience.... I'll either submit a patch or explain that I failed miserably and take the issue out of my name by next week.

Mile23’s picture

Keep in mind that there are a bunch of changes to the menu system, the most important being hook_menu() replaced with *.menu_link.yml: https://drupal.org/node/2223291

Also check the PHPUnit Example module for a template for testing menu links.

Thanks!

kgoel’s picture

Please note that this patch is old and there has been change in routing system including adoption of PSR-4.

mrjmd’s picture

Just wanted to post a quick update on my progress, which can currently be followed here: https://github.com/mrjmd/menu_example

Overall, I think most of this is functionally complete and working. I gutted the old module and previous patches because pretty much everything has changed. The only thing that remains at this point from the D7 version is the implementation of hook_permission()!

I'm going to continue working on this but here's what I know is still incomplete:

  • Custom Parameter Upcasting - The D7 menu_example module included a 'magic placeholder processing' example, which doesn't really have an equivalent in D8 as far as I've found. The closest equivalent I've seen is custom parameter upcasting, but even that is deemed rarely necessary in current documentation (entities are upcast automatically), so I'm not sure if an example of it is in scope or not. Thoughts? Either way, I haven't been able to get it to work yet.
  • Tests - I have not gotten to writing tests at all. Going to try to tackle this next week.
  • Documentation - If you look at my repo, you'll notice pretty much all code comments are gone right now. Partially that's because almost all of them need to be completely re-written, which I haven't done yet. But also, I'm not sure where the best place will be to put the new introductory comments. In the D7 version, there is pretty much just one big .module file, so the introduction starts at the top of that file and continues inline through the file. Since the new .module file is almost empty, and our file structure is much more complex, I'm not sure of the best place for our initial overview comments... should we include a README.txt instead or just assume people will still look at the .module file first?
  • Another thing worth mentioning / getting feedback on is the way the D7 version of this module was using page arguments to pass in the main content for many of the menu links. That approach is still possible in D8 routing files (and I included an example of it), but I've mostly abandoned that approach and instead implemented custom methods for each link. It means more methods in our controller but less markup in our routing file, which seems like better practice to me. Thoughts?

Once I get tests and documentation included I'll roll a patch, unless someone would like one now.

Also I'm keeping an eye on https://www.drupal.org/node/2256521; I think it's going to break a good bit of my work.

Mile23’s picture

  • Upcasting is definitely something to show off in this example.
  • As far as tests are concerned, there are a bunch from D7 in the patch at #11. You say the implementations have changed, but have the behaviors? Maybe roll those tests in and see what passes.
  • We want the main landing point for documentation to be the .module file. This is for consistency with all the other examples, including D7 and D6. The docblock @defgroup gets parsed into api.drupal.org, where the whole thing can be browsed. Any docblocks describing how to use the module once its installed, what each page/link/etc demonstrates, and pointers to show where to look for the things being demonstrated would be much appreciated. :-)
  • Putting methods on the controller is the right thing to do.

Also, wow... #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module is pretty major. Nothing like a last-minute re-architecting. :-)

Thanks.

Mile23’s picture

Assigned: mrjmd » Unassigned

Un-assigning. Please re-assign if you're still working on this.

Andrew.Mikhailov’s picture

Assigned: Unassigned » Andrew.Mikhailov
Andrew.Mikhailov’s picture

Assigned: Andrew.Mikhailov » Unassigned
Mile23’s picture

Basically the previous patches don't work because they don't use routes. We're also very close on #2277667: Port D7 Tests To D8 for menu_example which should be the baseline for behavior in this issue.

Mile23’s picture

Issue summary: View changes
Status: Needs work » Needs review
Parent issue: » #1880976: [meta] Port examples to D8
FileSize
5.05 KB

We have working failing tests now from #2277667: Port D7 Tests To D8 for menu_example

I've closed that issue and the patch here is the patch from that issue.

Proper git attribution for that issue:

git commit -m 'Issue #2277667 by mimran, snehi: Port D7 Tests To D8 for menu_example' --author="imran4uall <imran4uall@2712535.no-reply.drupal.org>"

Status: Needs review » Needs work

The last submitted patch, 23: examples-menu-example-test-2277667-28-8.x.patch, failed testing.

AbhishekLal’s picture

Assigned: Unassigned » AbhishekLal

I am creating fresh patch for this

AbhishekLal’s picture

Status: Needs work » Needs review
FileSize
14.11 KB

Ported few files.

Status: Needs review » Needs work

The last submitted patch, 26: menu-examples-1883724-26.patch, failed testing. View results

AbhishekLal’s picture

Well, I've ported some of the examples, But I am still facing some issues which are
1) Creating the primary and Secondary tab.
2)Processed Placeholder Arguments.
3)Title Callback, instead of username I was only able to show userid.
4)Ports for the callback only , custom access menu item are not made.

AbhishekLal’s picture

Well, It is a working patch of Menu example. @navneet0693 and @vaibhavjain thank you for the guidance.

AbhishekLal’s picture

Status: Needs work » Needs review

The last submitted patch, 11: 2277667_do_not_test.patch, failed testing. View results

The last submitted patch, 28: menu_example-1883724-27.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 29: menu_example-1883724-28.patch, failed testing. View results

AbhishekLal’s picture

Status: Needs work » Needs review
FileSize
22.79 KB
Mile23’s picture

Status: Needs review » Needs work

Thanks, @AbhishekLal :-)

There are probably other coding standards issues, but let's start here:

  1. +++ b/menu_example/menu_example.links.menu.yml
    @@ -0,0 +1,77 @@
    +  ¶
    ...
    +  ¶
    
    +++ b/menu_example/menu_example.permissions.yml
    @@ -0,0 +1,7 @@
    +
    +
    
    +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,284 @@
    +
    
    +++ b/menu_example/tests/menu_example/functional/MenuExampleTest.php
    @@ -0,0 +1,106 @@
    +
    +
    

    Bunch of white space errors.

  2. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,284 @@
    +    return [
    +      '#markup' => $this->t('This page is displayed by the simplest (and base)
    +       menu example. Note that the title of the page is the same as the link
    +       title. You can also @link. There are a number of
    +       examples here, from the most basic (like this one) to extravagant
    +        mappings of loaded placeholder arguments. Enjoy!', ['@link' => $link]),
    +    ];
    

    This should use a translatable template. See BlockExampleController (and block_example/templates/description.html.twig)

  3. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,284 @@
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    ...
    +  /**
    +   *
    +   */
    

    These docblocks should say something. :-) https://www.drupal.org/docs/develop/coding-standards/api-documentation-a... Explain what the controller method is demonstrating.

  4. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,284 @@
    +/**
    +   *
    +   */
    

    Extraneous docblock.

  5. +++ b/menu_example/tests/menu_example/functional/MenuExampleTest.php
    @@ -0,0 +1,106 @@
    +namespace Drupal\tests\menu_example\functional;
    

    These tests aren't run because they use the wrong PSR-4 file mapping. This test file should be in /menu_example/tests/src/Functional/MenuExampleTest.php

    That should map to the namespace Drupal\Tests\menu_example\Functional

  6. +++ b/menu_example/tests/menu_example/functional/MenuExampleTest.php
    @@ -0,0 +1,106 @@
    +  /**
    +   * test whether the module was installed.
    +   */
    +  public function testInstalled() {
    +    $this->assertTrue(\Drupal::moduleHandler()->moduleExists('menu_example'));
    +  }
    

    Remove this method. The other tests will fail if the module is not installed.

navneet0693’s picture

Hey @AbhishekLal thats a great work and a lot of work :) thanks for you efforts !!

Adding to what @Mile23 pointed to, I am adding few points here :)

  1. +++ b/menu_example/menu_example.links.menu.yml
    @@ -0,0 +1,77 @@
    +  ¶
    ...
    +  ¶
    

    Extra white space.

  2. +++ b/menu_example/menu_example.permissions.yml
    @@ -0,0 +1,7 @@
    +
    

    Extra line.

  3. +++ b/menu_example/menu_example.routing.yml
    @@ -0,0 +1,184 @@
    +  path: '/examples/menu_example/permissioned'
    ...
    +  path: '/examples/menu_example/permissioned/controlled'
    ...
    +  path: '/examples/menu_example/custom-access'
    ...
    +  path: '/examples/menu_example/custom-access/page'
    ...
    +  path: '/examples/menu_example/path-only'
    ...
    +  path: '/examples/menu_example/path-only/callback'
    ...
    +  path: 'examples/menu_example/tabs'
    ...
    +  path: 'examples/menu_example/tabs/default'
    ...
    +  path: 'examples/menu_example/tabs/second'
    ...
    +  path: 'examples/menu_example/tabs/third'
    ...
    +  path: 'examples/menu_example/tabs/forth'
    ...
    +  path: 'examples/menu_example/tabs/default/Second'
    ...
    +  path: 'examples/menu_example/tabs/default/third'
    ...
    +  path: 'examples/menu_example/use-url-arguments/{arg1}'
    ...
    +  path: 'examples/menu_example/use-url-arguments/{arg1}/{arg2}'
    ...
    +  path: 'examples/menu_example/title-callbacks'
    ...
    +  path: 'examples/menu_example/placeholder-argument'
    ...
    +  path: 'examples/menu_example/placeholder-argument/{node}/display'
    ...
    +  path: 'examples/menu_example/default_arg/{no}'
    ...
    +  path: 'examples/menu_example/menu_original_path'
    

    //s /examples/menu_example/permission to /examples/menu-example/permission to main consistency :) and also Drupal 8 use - instead of _ in paths.

  4. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,284 @@
    +      '#markup' => t('This will be in the Main menu instead of the default Tools menu'),
    

    Use $this->t() instead of t()

  5. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,284 @@
    +    $urle = Url::fromUri('internal:/examples/menu_example/use-url-arguments/one/two');
    +    $linker = Link::fromTextAndUrl($this->t('examples/menu_example/use-url-arguments/one/two'), $urle)->toString();
    

    More meaningful variable names will look good :)

  6. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,284 @@
    +      '#markup' => $this->t('Argument1 = @arg1 , Argument2 = @arg2', ['@arg1' => $arg1, '@arg2' => $arg2]),
    

    Use % for replacement here.

    Read more about placeholders here: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Rend...

  7. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,284 @@
    +    // $title = $account->getUsername()->toString();
    +    // $account = \Drupal::currentUser()->id();
    +    // $title=$account->get('name')->value;
    

    Commented code ???

  8. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,284 @@
    +      '#markup' => $this->t('This menu item was created strictly to allow the hook_menu_alter() function to have something to operate on.hook_menu defined the path as examples/menu_example/menu_original_path. The hook_menu_alter() changes it to examples/menu_example/menu_altered_path. You can try navigating to both paths and see what happens!'),
    

    //s on.hook_menu to on hook_menu

    We might need to update description ?

  9. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,284 @@
    +
    

    Extra line, this can be utilized to leave description.

  10. +++ b/menu_example/tests/menu_example/functional/MenuExampleTest.php
    @@ -0,0 +1,106 @@
    +
    +
    +  }
    

    One line before function ending bracket and one after it will look good.

AbhishekLal’s picture

@Mile23 and @navneet0693 I was able to solve some issues you guys have mentioned, I had a tough time with 2nd one in #37 and some are yet to be completed.. This is a working patch..

Mile23’s picture

Status: Needs work » Needs review

Setting needs review to run the tests.

navneet0693’s picture

Assigned: AbhishekLal » navneet0693

Adding few things, I will upload a patch soon.

navneet0693’s picture

AbhishekLal’s picture

The patch applied properly and working correctly, thank you so much.

Mile23’s picture

Status: Needs review » Needs work

If you check the console output of the last test run, you'll see that no menu_example tests ran: https://dispatcher.drupalci.org/job/drupal8_contrib_patches/8470/console

navneet0693’s picture

Mile23 is right, I must have missed something :-( .

navneet0693’s picture

Status: Needs work » Needs review
FileSize
9.01 KB
24.11 KB

Status: Needs review » Needs work

The last submitted patch, 44: menu_example-1883724-44.patch, failed testing. View results

navneet0693’s picture

Status: Needs work » Needs review
FileSize
636 bytes
24.12 KB

Status: Needs review » Needs work

The last submitted patch, 46: menu_example-1883724-46.patch, failed testing. View results

Mile23’s picture

+++ b/menu_example/tests/src/Functional/MenuExampleTest.php
@@ -0,0 +1,90 @@
+  protected $profile = 'minimal';
...
+    $this->assertText('Menu Example');

I think the problem is that the minimal profile doesn't show the tools menu block. Though I could be remembering incorrectly.

navneet0693’s picture

@Mile23 I am not sure if that is the problem. Quoting the comment from queue_example and also procedure is same as in other examples.

* We need the 'minimal' profile in order to make sure the Tool block is
* available.

Mile23’s picture

navneet0693’s picture

navneet0693’s picture

Status: Needs work » Needs review
navneet0693’s picture

@Mile23, and now we know why the tests were failing, I was able to produce this when i installed drupal core dev (8.4.x) and ran the test. Somehow title were being read from .links.menu.yml in 8.3.x which were without quotes ( see interdiff.txt ) but not on 8.4.x !

Thanks your hint on IRC, i was able to solve this.

This patch is ready for review.

Mile23’s picture

Status: Needs review » Needs work

Nice.... Getting closer. :-)

  1. +++ b/menu_example/menu_example.info.yml
    --- /dev/null
    +++ b/menu_example/menu_example.links.menu.yml
    
    +++ b/menu_example/menu_example.links.menu.yml
    --- /dev/null
    +++ b/menu_example/menu_example.links.task.yml
    
    +++ b/menu_example/menu_example.module
    --- /dev/null
    +++ b/menu_example/menu_example.permissions.yml
    
    +++ b/menu_example/menu_example.permissions.yml
    --- /dev/null
    +++ b/menu_example/menu_example.routing.yml
    

    All of these YAML files need a ton of comments to explain the relationships between the different items.

    For instance, the tab examples need to explain how they're set up as tabs.

  2. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,255 @@
    +  public function defaultSecond() {
    ...
    +  public function defaultThird() {
    

    There has to be some way to do this with introversion, so that all the secondary tabs can be one method that knows which tab it should represent.

  3. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,255 @@
    +  /**
    +   * Demonstrate generation of dynamic creation of page title.
    +   */
    +  public function titleCallback() {
    ...
    +  /**
    +   * Generates title dynamically.
    +   */
    +  public function backTitle() {
    

    These methods need to @see each other. They also need to explain that they're connected by the yaml specification.

  4. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,255 @@
    +      '#markup' => $this->t('The title of this page is dynamically changed by the title callback for this route. Also, the title of the menu link is dynamically changed by implementing hook_menu_link_presave() and hook_translated_menu_link_alter().'),
    

    This statement isn't accurate. We specify a title callback in the yaml.

  5. +++ b/menu_example/src/Controller/MenuExampleController.php
    @@ -0,0 +1,255 @@
    +  /**
    +   * Displays placeholder argument supplied in URL.
    +   *
    +   * @param int $arg
    +   *   URL argument.
    +   *
    +   * @return array
    +   *   URL argument.
    +   */
    +  public function placeholderArgsDisplay($arg) {
    +    return [
    +      '#markup' => $arg,
    +    ];
    

    This kind of thing creeps me out. :-) Are we sure that sanitization is applied somewhere else?

  6. +++ b/menu_example/src/Routing/RouteSubscriber.php
    @@ -0,0 +1,24 @@
    +/**
    + * Listens to the dynamic route events.
    + */
    +class RouteSubscriber extends RouteSubscriberBase {
    

    Should explain what the route subscriber is listening for, and what it will accomplish.

  7. +++ b/menu_example/templates/description.html.twig
    @@ -0,0 +1,14 @@
    +<p>This page is displayed by the simplest (and base) menu example. Note that the title of the page is the same as the link title. You can also <a href="{{ link }}">visit a similar page with no menu link</a>.
    +There are a number of examples here, from the most basic (like this one) to extravagant mappings of loaded placeholder arguments. Enjoy!</p>
    

    Wrap to 80.

  8. +++ b/menu_example/tests/src/Functional/MenuExampleTest.php
    @@ -0,0 +1,90 @@
    + * @ingroup menu-example
    ...
    + * @group menu-example
    

    menu_example

  9. +++ b/menu_example/tests/src/Functional/MenuExampleTest.php
    @@ -0,0 +1,90 @@
    +  public function testMenuExample() {
    

    It seems like we've defined more routes than are tested here.

Mile23’s picture

navneet0693’s picture

Status: Needs work » Needs review
FileSize
24.06 KB
31.37 KB
navneet0693’s picture

@Mile23 For point 5: We are using following, which I think should do the work.

requirements:
    arg: \d+

Status: Needs review » Needs work

The last submitted patch, 56: menu_example-1883724-56.patch, failed testing. View results

navneet0693’s picture

Status: Needs work » Needs review
FileSize
4.57 KB
32.45 KB

I am sorry, I forgot to update tests, that's why previous tests got failed.

AbhishekLal’s picture

Patch applied properly and working great!!