Skip to content

[WIP] Support initializer priority #173

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

Merged
merged 1 commit into from
Oct 25, 2014

Conversation

dantleech
Copy link
Contributor

Made a start. Unfortunately I don't seem to be able to install the depdenencies and run the tests ..

@dantleech
Copy link
Contributor Author

@lsmith77 which issue does this relate to?

@lsmith77
Copy link
Member

@@ -90,4 +97,32 @@ public function initialize()
$initializer->init($this->registry);
}
}


Copy link
Member

Choose a reason for hiding this comment

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

extra new line

@lsmith77 lsmith77 added this to the 1.2 milestone Oct 25, 2014
{
$this->initializers[] = $initializer;
$this->initializers[] = array($initializer, $priority);
Copy link
Member

Choose a reason for hiding this comment

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

huh? i think you need to remove this line. otherwise you add every initializer twice, once at some undetermined prio starting with 0, and then with the explicit prio.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WIP (tm)

@dantleech dantleech force-pushed the support_priority_intializer branch from f21c59d to 4769b27 Compare October 25, 2014 13:45
@@ -50,8 +50,14 @@ public function process(ContainerBuilder $container)
throw new LogicException(sprintf('initializer "%s" must be public', $id));
}

$priority = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems wrong to define this in two places, i.e. as the default value of the addInitializer method and here. But the other options I can see are doing an array_call_user_func or conditionally two versions of the addMethodCall line below.

Copy link
Member

Choose a reason for hiding this comment

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

given that its all in the same class imho its fine. but if you really wanted to, i guess a class constant could work though IIRC you cannot default to a const, so you would need to default to null and check for it .. imho not worth the trouble.

@dantleech dantleech force-pushed the support_priority_intializer branch from 4769b27 to 6f29b41 Compare October 25, 2014 13:56
@dantleech dantleech force-pushed the support_priority_intializer branch from 6f29b41 to 2e31ffd Compare October 25, 2014 13:57
@dantleech
Copy link
Contributor Author

Updated. Tested with the RoutingAuto tutorial that I started this morning and all is well.

@lsmith77
Copy link
Member

ca you rebase on master to clean up the travis results?

@lsmith77
Copy link
Member

ok never mind .. I am testing this and will merge and tag then.

lsmith77 added a commit that referenced this pull request Oct 25, 2014
@lsmith77 lsmith77 merged commit 1a4fc66 into doctrine:master Oct 25, 2014
wouterj added a commit to symfony-cmf/symfony-cmf-docs that referenced this pull request Oct 26, 2014
This PR was submitted for the dev branch but it was merged into the master branch instead (closes #595).

Discussion
----------

[WIP] Tutorial Updates for 1.2

Updates and improvements for 1.2.

Note there is a problem in that the RoutingAutoBundle intializes before the initializer that we create in the last chapter, causing it to fail.

We need to add support for initializer priority: doctrine/DoctrinePHPCRBundle#173

Commits
-------

dc3de60 [WIP] Tutorial Updates for 1.2
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.

3 participants