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

Readme and container calling #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Readme and container calling #1

wants to merge 3 commits into from

Conversation

adamculp
Copy link

@adamculp adamculp commented Sep 9, 2013

Updates to Readme to be more basic for beginning users, and also updated templates to call the container name for navigation array from the config.

@juriansluiman
Copy link

Thanks for the PR! I have to test whether the explicit 'navigation' container name still works in our case. If so, I'll merge it soon.

@juriansluiman
Copy link

@adamculp I have tested it now. It seems when I inject the navigation manually (which is the case with ensemble, our cms) the explicit navigation container raises an exception.

My suggestion: create a ModuleOptions class which contains a "navigation_key" (or so) option. Can be set in the config files (namespaced soflomo_sitemap) and injected into the constructor. Then given to the view model and used in the view helper call.

This makes it possible to create a sitemap of any navigation structure, and leaving it null enables sitemaps for ensemble-style injection.

If you don't have the time or understand what I mean, I'll make a proposal for you.

@adamculp
Copy link
Author

I see the difference, I think. The method I used was to add a 'navigation' container through the Application module.config.php and naming the array. (As found in the Quick Start http://framework.zend.com/manual/2.2/en/modules/zend.navigation.quick-start.html) Since I started with Navigation in mind and not merely a sitemap.

It seems the method you are using is building a container to use as ModuleOptions class, though it was not covered in your documentation.

So, I guess the question is...which method should we use for this project. (I see more flexibility with the method I used because it allows multiple containers with different names, so different menu navigation can be created within one application.

Please advise.

@juriansluiman
Copy link

Adam, I think you misunderstood me :)

What I meant in the view script, I'd rather have a single line changed. It was (before your PR) this:

<?= $this->navigation()->menu()

You made it this:

<?= $this->navigation('navigation')->menu()

I am now suggesting this:

<?= $this->navigation($container)->menu()

Where $container is a string value or null which gets injected from the controller:

class SitemapController extends AbstractActionController
{
    protected $options;

    public function __construct(ModuleOptions $options)
    {
        $this->options = $options;
    }

    public function indexAction()
    {
        return new ViewModel(array(
            'container' => $this->options->getContainerName()
        ));
    }

    public function xmlAction()
    {
        // Explicitly set type to text/xml, otherwise it's text/html
        $this->getResponse()->getHeaders()->addHeaderLine(
            'Content-Type', 'text/xml'
        );

        $viewModel = new ViewModel(array(
            'container' => $this->options->getContainerName()
        ));

        // Only render the sitemap helper, without any layout
        $viewModel->setTerminal(true);
        return $viewModel;
    }
}

So you have a configuration which might look like this:

return array(
    'soflomo_sitemap' => array(
        'container' => null, // Or 'navigation' in your case
    ),
);

This allows you to have much more flexibility over what container you want to display. I can imagine we'll get options for renderInvisible etc now too.

@adamculp
Copy link
Author

Ah, I see it now. Thanks for explaining. I will implement the change and add to the PR.

One thing I'm still unclear on. In looking more closely I am not sure this would be a good option. (Please let me know if I am still missing something.) How will $options be injected to the SitemapController?

@adamculp
Copy link
Author

I updated previous comment.

@juriansluiman
Copy link

Good questions :) I have some examples for you:

  • The ModuleOptions class can inherit from the AbstractOptions as also shown in this example
  • You can create a module options class with a factory and register this factory in the service config.
  • Then create a controller factory where the options class is pulled from and injected. The controller must be registered as factory and thus the invokable must be removed

As I said, I can do this too, but it would be great if you are willing to help!

@adamculp
Copy link
Author

Thanks for the notes, and patience to help me while I learn ZF2. I will review what you wrote.

@juriansluiman
Copy link

The general idea here I'd like to follow is to inject the options into the controller (as you already pointed out). In order to achieve this, you cannot use the controller as invokable, but you must use a factory.

The same holds for the options class. The options must be configured with a config from the service manager, so you need a factory for that one too. You glue it all together with the service manager to create the object graph. If you have any questions, just ask me :)

@juriansluiman
Copy link

@adamculp This issue is open for quite a while now. I have had similar questions about it in #2 and #3. Have you had time to update this? Otherwise, @phpjob might give a hand or I make some time to get this done.

@adamculp
Copy link
Author

Yes, it has been awhile. Got very busy and forgot about it. I will review and let you know.

@juriansluiman
Copy link

Adam, no problem :) I forgot it as well and as I got the other issues, I stumbled upon this one again :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants