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

[3.0.x] Remove $di being passed in as second parameter into Volt service #11926

Closed
dschissler opened this Issue Jun 26, 2016 · 17 comments

Comments

Projects
4 participants
@dschissler
Copy link
Contributor

dschissler commented Jun 26, 2016

Phalcon 2.1 is binding the service functions to the dependency injector and so the second parameter being passed into the Volt definition is unnecessary.

This would turn the following:

$di->set('voltService', function($view, $di) {
    ...
    $volt = new Volt($view, $di);

into this:

$di->set('voltService', function($view) {
    ...
    $volt = new Volt($view, $this);

This would add consistency to the services but would have the downside of breaking existing service definitions.

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Jun 26, 2016

It seems to me this is mainly phalcon/docs issue

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Jun 28, 2016

We just need to implemt both behaviours and that's it.

@dschissler

This comment has been minimized.

Copy link
Contributor Author

dschissler commented Jun 28, 2016

Both behaviours are already implemented. This issue is about removing one of them. I don't want to close this issue because I think that it should be this way but since its just a matter of opinion then somebody else can close it.

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Jun 28, 2016

Could you please explain guys what kind of problem you are discussing

@dschissler

This comment has been minimized.

Copy link
Contributor Author

dschissler commented Jun 28, 2016

I am not having a problem in the sense that I can't get it to do something or that there is regression. Since I've been using the unreleased 2.1.x branch I have been taking advance of new features and capabilities. One of those advancements is not needing to setup services in ways like function() use ($di) since with Phalcon 2.1 the service function is bound to the DI that it is being attached to. I see it that the current voltService (maybe all engines, not sure?) signature is accepting a DI second parameter but this is now unnecessary sine the service is itself bound to the DI. It has been suggested elsewhere that this allows for the use of two DIs but this was never a particular use case for the views and template engine throughout Phalcon's history so I think that it should be considered an artefact of old limitations (needing to support PHP 5.3 without Closure:: tech). So this is just something like an API cleanup.

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Jun 28, 2016

For me old syntax shouldn't be removed on version 2.x. We could do it on 3.x for me. Further 2.x shouldn't have any code breaking previous 2.x versions. There should be as little as possible changes in api to update to new minor version. We should just deprecate old syntax(just trigger E_DEPRECATED if it's used) but it should work in my opinion as it is.

@dschissler

This comment has been minimized.

Copy link
Contributor Author

dschissler commented Jun 28, 2016

I don't think that there is a way to deprecate it in this case because it is an extra argument being sent in.

Ok so version 3 is super far away. Should this just be closed?

Also since Phalcon is not semver now we could just remove this in version 2.2 or 3.0. It would be very easy to document in a blog whats new post and quickly people would use the new way which is compatible with 2.1.

@dschissler dschissler closed this Jun 28, 2016

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Jun 28, 2016

Where there is - we just need to use reflection, but should we ? Just if function accept two arguments - trigger deprecated and inform in which version it will be removed. We could in 2.2 maybe beacause we don't follow semver. But i think we always should inform about something before remove it, not everyone will check changelog, github or blog. Some of people will just upgrade and that's it. At least in logs they could see something.

@dschissler

This comment has been minimized.

Copy link
Contributor Author

dschissler commented Jun 28, 2016

Ok then we should probably do that.

@dschissler dschissler reopened this Jun 28, 2016

@dschissler

This comment has been minimized.

Copy link
Contributor Author

dschissler commented Oct 14, 2016

Just for reference sake Vokuro has been using the new 3.0 capable style for a few months now without issue: https://github.com/phalcon/vokuro/blob/9573515fee52aceb7873762ecc12a5dd2c9ef554/app/config/services.php#L52

@dschissler dschissler changed the title [2.1.x] Remove $di being passed in as second parameter into Volt service [3.0.x] Remove $di being passed in as second parameter into Volt service Nov 5, 2016

@dschissler

This comment has been minimized.

Copy link
Contributor Author

dschissler commented Nov 5, 2016

Phalcon switched to semver when it jumped to version 3.0. I propose that in late 2017 or so that a minor release should start throwing a deprecation warning if the second parameter is used.

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Nov 5, 2016

$di->setShared(
    'volt',
    function (\Phalcon\Mvc\ViewBaseInterface $view, \Phalcon\DiInterface $di = null) {
        /** @var \Phalcon\DiInterface $this */
        $config = $this->getShared('config');

        $volt = new Volt($view, $di ?: $this);

        $volt->setOptions(
            [
                'compiledPath'      => $config->application->view->compiledPath,
                'compiledSeparator' => $config->application->view->compiledSeparator,
                'compiledExtension' => $config->application->view->compiledExtension,
                'compileAlways'     => (bool) $config->application->debug,
            ]
        );

        return $volt;
    }
);


$di->setShared(
    'view',
    function () use () {
        /** @var \Phalcon\DiInterface $this */
        $config = $this->getShared('config');

        $view = new \Phalcon\Mvc\View();

        $engines = [
          '.volt' => $this->getShared('volt', [$view, $this]),
        ];

        $view->registerEngines($engines);
        $view->setViewsDir($config->application->view->viewsDir);

        return $view;
    }
);

@sergeyklay sergeyklay closed this Nov 5, 2016

@dschissler

This comment has been minimized.

Copy link
Contributor Author

dschissler commented Nov 6, 2016

My point wasn't that this should be closed or that it is impossible to make a DI service now, but that removing the DI being sent in for the second parameter would break existing stuff. So its pretty harmless for now and it needs to be there for the entire 3.* series but eventually it (DI being passed in as 2nd argument) should be removed for the 4 series.

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Nov 6, 2016

It can be done in 3.* with keeping BC using reflection but there is no point in this imho.

@dschissler

This comment has been minimized.

Copy link
Contributor Author

dschissler commented Jan 10, 2019

@sergeyklay @niden I propose that you reopen this and remove the $di second argument being passed for the volt function. This is simply an artifact a long ago time when the DI service functions weren't bound to the DI object. This is just inconsistent API and is not thoughtful or useful, its just old stuff that was never cleaned up. The second parameter in the function does not need to be there. Just get the DI from $this. Any special weird setup that is precompiling or other unusual cases can simply bind the volt service function to the DI. I don't believe that there is a use case for passing in two different DIs, (one from $this and one from the second parameter). That doesn't exist. Clean this crap up please.

@dschissler

This comment has been minimized.

Copy link
Contributor Author

dschissler commented Jan 10, 2019

I propose just removing the di and not doing reflection as jurigag proposed before to save time adding problematic fix code. It will be slightly confusing at first but its super easy to figure out during an upgrade. The error logs will already say (expected DI but got null). They'll figure it out real quickly.

@sergeyklay sergeyklay reopened this Jan 10, 2019

@sergeyklay sergeyklay added this to To do in 4.0 Release via automation Jan 10, 2019

@sergeyklay sergeyklay added this to the 4.0.0 milestone Jan 10, 2019

dschissler added a commit to dschissler/cphalcon that referenced this issue Feb 18, 2019

[phalcon#11926] Changed view engine service closures to no longer rec…
…eive the dependency injector as the second parameter.
@niden

This comment has been minimized.

Copy link
Member

niden commented Feb 23, 2019

Closing in favor of #13855. Will revisit if the community votes for it, or in later versions.

@niden niden closed this Feb 23, 2019

4.0 Release automation moved this from To do to Done Feb 23, 2019

niden added a commit that referenced this issue Feb 24, 2019

[#11926] Changed view engine service closures to no longer receive th… (
#13839)

* [#11926] Changed view engine service closures to no longer receive the dependency injector as the second parameter.

* Attempt to fix Volt engine integration test.

* Bind view service closures to the DI, fix more tests.

* Update CHANGELOG-4.0.md

niden added a commit to niden/cphalcon that referenced this issue Feb 25, 2019

[phalcon#11789] - Merge remote-tracking branch 'upstream/4.0.x' into …
…T11789-PSR-7

* upstream/4.0.x:
  Fixed phalcon#13858 - Nanobox config fails for php 7.3 (phalcon#13866)
  Remove all 'fresh' code from the DI (since its no longer needed for the dispatcher hack). (phalcon#13861)
  Update boxfile.7.2.yml (phalcon#13859)
  Refactored security tests from SecurityCest (phalcon#13864)
  Cleanup
  Again regenerating ext/
  Regenerating ext folder
  DI::get shared caching - a more comprehensive solution (phalcon#13846)
  Allow grouped config loader to determine adaptor from file extension. (phalcon#13762)
  Dispatcher handlercache (phalcon#13854)
  [phalcon#13835] - Changed Volt compiler options. (phalcon#13840)
  [phalcon#11926] Changed view engine service closures to no longer receive th… (phalcon#13839)
  Corrected typo

niden added a commit to niden/cphalcon that referenced this issue Mar 3, 2019

[4.0.x] - Merge remote-tracking branch 'upstream/4.0.x' into 4.0.x
* upstream/4.0.x:
  Regenerated the build
  Regenerated ext
  Cleaned not used variable
  Update change log
  typo in comment
  Pdo radicaloptions (phalcon#13862)
  Fixed phalcon#13858 - Nanobox config fails for php 7.3 (phalcon#13866)
  Remove all 'fresh' code from the DI (since its no longer needed for the dispatcher hack). (phalcon#13861)
  Update boxfile.7.2.yml (phalcon#13859)
  Refactored security tests from SecurityCest (phalcon#13864)
  Cleanup
  Again regenerating ext/
  Regenerating ext folder
  DI::get shared caching - a more comprehensive solution (phalcon#13846)
  Allow grouped config loader to determine adaptor from file extension. (phalcon#13762)
  Dispatcher handlercache (phalcon#13854)
  [phalcon#13835] - Changed Volt compiler options. (phalcon#13840)
  [phalcon#11926] Changed view engine service closures to no longer receive th… (phalcon#13839)
  Corrected typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.