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

[BUG]: Passing variables from the controller to the view does not work. #14491

Closed
Roninju opened this issue Oct 26, 2019 · 22 comments
Closed

[BUG]: Passing variables from the controller to the view does not work. #14491

Roninju opened this issue Oct 26, 2019 · 22 comments

Comments

@Roninju
Copy link

@Roninju Roninju commented Oct 26, 2019

Describe the bug
There seems to be a communication problem between the controller and the view that causes the variables not to be defined in the view.

This problem happens since version 4.0.0-rc.2+4138

To Reproduce

use Phalcon\Mvc\Controller;

class IndexController extends Controller {
    
    public function indexAction() {
		
	$this->view->setVar('test', 'value'); // This does not work in view
	$this->view->disable(); // This does not work.
	echo $this->view->getVar('test'); // This works in controller.
		
    }
	
}

Details

  • Phalcon version: 4.0.0-rc.2+4237 x64 nts
  • PHP Version: PHP 7.3.10 x64 nts
  • Operating System: Windows 10 x64
  • Server: Apache
@sergeyklay

This comment has been minimized.

Copy link
Member

@sergeyklay sergeyklay commented Oct 27, 2019

@Roninju Did you use PHP FPM?

P.S. Are you sure 4.0.0-rc.1 is OK?

@ekmst

This comment has been minimized.

Copy link
Contributor

@ekmst ekmst commented Oct 27, 2019

The following also does not work for me:

use Phalcon\Mvc\Controller;

class IndexController extends Controller
{
    public function indexAction()
    {		
	$this->view->testVar = 'value';
    }	
}

and output:

Notice: Undefined variable: testVar in ...views/index/index.phtml on line ...

Details:

  • Phalcon Version: 4.0.0-rc.2
    • Build Date: Oct 26 2019 19:34:10
  • PHP Version: 7.3.9
  • OS: linux
@Roninju

This comment has been minimized.

Copy link
Author

@Roninju Roninju commented Oct 27, 2019

@sergeyklay I don't use PHP-fpm.

4.0.0-rc.1 works correctly. The latest version that does not have this problem is phalcon_x64_vc15_php7.3_4.0.0-rc.2+4137_nts.

@sergeyklay

This comment has been minimized.

Copy link
Member

@sergeyklay sergeyklay commented Oct 27, 2019

@ekmst Do you use Apache too? What about PHP SAPI, is it PHP-FPM?

@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Oct 27, 2019

@Roninju @ekmst Can you share how your view is setup in the DI?
Are you using set or setShared() ?

@ekmst

This comment has been minimized.

Copy link
Contributor

@ekmst ekmst commented Oct 27, 2019

@sergeyklay
No, I use Nginx in conjunction with FPM

@Jeckerson

This comment has been minimized.

Copy link
Member

@Jeckerson Jeckerson commented Oct 27, 2019

@ekmst Try to declare view service provider with setShared()
Example here:
https://github.com/phalcon/vokuro/blob/4.0.x/src/Providers/ViewProvider.php#L40

@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Oct 27, 2019

Confirmed the bug. It works on RC.1 and 3.4. Looks like we reinitialise the View somewhere.
Workaround is defining your view as shared.

// Registering the view component
$di->setShared(
    'view',
    function () {
        $view = new View();
        // Settings
        return $view;
    }
);
@Jeckerson

This comment has been minimized.

Copy link
Member

@Jeckerson Jeckerson commented Oct 27, 2019

After tests with Vokuro

RC.1

Work

$di->set('view', ...)

Work

$di->setShared('view', ...)

RC.2

Doesn't work

$di->set('view', ...)

Work

$di->setShared('view', ...)
@ekmst

This comment has been minimized.

Copy link
Contributor

@ekmst ekmst commented Oct 27, 2019

@Jeckerson
I have the following

// view service (fpm)
$view = new \Phalcon\Mvc\View();

$view->setViewsDir(__DIR__ . '/views/');
$view->setPartialsDir('_partials/');
$view->setLayoutsDir('_layouts/');

$di->set('view', $view');       // Doesn't work

$di->setShared('view', $view);  // But it works
@ekmst

This comment has been minimized.

Copy link
Contributor

@ekmst ekmst commented Oct 27, 2019

@sergeyklay @ruudboon @Jeckerson

Here is what I noticed. Views do not work when using closure. Below are examples of code and their behaviors in my environment:

It works

    public function registerServices(DiInterface $di): void
    {
        $view = new View();
        $view->setViewsDir(__DIR__ . '/views/');
        $di->set('view', $view);        // It works
        // or
        $di->setShared('view', $view);  // It works
    }

It works

    public function registerServices(DiInterface $di): void
    {
        $di->setShared('view', function () {    // It works
            $view = new View();
            $view->setViewsDir(__DIR__ . '/views/');
            return $view;
        });
    }

Doesn't work

    public function registerServices(DiInterface $di): void
    {
        $di->set('view', function () {   // Doesn't work
            $view = new View();
            $view->setViewsDir(__DIR__ . '/views/');
            return $view;
        });
    }
@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Oct 27, 2019

@ekmst Does your second example work? In that case I think it's only happening with set and not related to the closure.

@ekmst

This comment has been minimized.

Copy link
Contributor

@ekmst ekmst commented Oct 27, 2019

@ruudboon Added descriptions

@Jurigag

This comment has been minimized.

Copy link
Member

@Jurigag Jurigag commented Oct 29, 2019

To be honest i would consider if this is really a bug. setShared is expected to return the same instance of a service while set not. $di->set('view', $view); actually works because we created instance of class earlier and we are setting the same instance to view. However using closure we are creating new instance each time, causing set to return new each time as expected.

In comparison of how it worked previously, yea, it's a bug, but was this previous behavior correct?

@alexbusu

This comment has been minimized.

Copy link
Contributor

@alexbusu alexbusu commented Oct 29, 2019

What @Jurigag says makes sense. But anyway, this issue... it smells like a non-optimal design to me at all.
The most common scenario is to getShared service (and setShared as well).
So why not use get/set with "shared instances" in mind (like singleton pattern), and use factory/fresh methods instead, to get "new instances" for queried services?
This does not change the current functionality, but only requires to rename or remove some methods which hopefully will make the things more clear, and less prone to generate confusion.

@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Oct 29, 2019

There are cases where you want the view as non shared (for example when using the view to render email templates as well).
Internally we get the view using getShared('view') this will create a shared instance (second call to getShared('view')` will return same view instance.
Currently debugging this behaviour.

@niden

This comment has been minimized.

Copy link
Member

@niden niden commented Oct 29, 2019

I agree with @Jurigag and @alexbusu The way the di was designed is indeed not optimal and misleading in cases. The get should always give me the same shared object and if I want a brand new one I can use factory/fresh to get that new object. This is the way psr-11 containers operate also.

I think once we release a full psr-11 container things will start becoming a bit easier. For now we just have to work with what we have. :/

@ruudboon ruudboon mentioned this issue Oct 29, 2019
3 of 5 tasks complete
@v00v

This comment has been minimized.

Copy link

@v00v v00v commented Oct 29, 2019

Please make it so $di->set() works too. Why do we have to use $di->setShared()? This should be a default! Thank you!

@niden

This comment has been minimized.

Copy link
Member

@niden niden commented Oct 29, 2019

Please make it so $di->set() works too. Why do we have to use $di->setShared()? This should be a default! Thank you!

It is in the future plans after v4 is released. We intend on releasing a new container fully PSR-11 compatible and with auto-wiring.

We cannot change this behavior now because it will pretty much break every single application there is out there that is to be upgraded to v4. By releasing a new container we give developers ample time to adjust their applications.

@v00v

This comment has been minimized.

Copy link

@v00v v00v commented Oct 29, 2019

Why did it show a BLANK PAGE and no error was given when $di->set('view') was used in RC2? It should of trown some error about this. If this was not known before, it should be now. To give some sort of error about a view in RC3 at least. Once you fully write PSR-11 this errors can be removed later. This was my main confusion about this, becauwe I could not see any errors, just a blank page from RC1 to RC2.

@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Oct 30, 2019

@v00v Error is in your error log.

@ruudboon ruudboon mentioned this issue Nov 4, 2019
4 of 5 tasks complete
@niden niden added this to To do in 4.0 Release via automation Nov 4, 2019
@niden niden added the 4.0 label Nov 4, 2019
niden added a commit that referenced this issue Nov 4, 2019
@niden

This comment has been minimized.

Copy link
Member

@niden niden commented Nov 4, 2019

Resolved in #14513

@Roninju can you build Phalcon from the 4.0.x branch and test this just in case please?

@niden niden moved this from To do to In progress in 4.0 Release Nov 6, 2019
@niden niden closed this Nov 11, 2019
4.0 Release automation moved this from In progress to Done Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4.0 Release
  
Done
9 participants
You can’t perform that action at this time.