[ZF3] FQCN in parameter type hints #4149

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
7 participants
Member

Ocramius commented Mar 30, 2013

This PR introduces FQCNs for type-hints in generated method signatures. This is a BC break, but it is necessary to use the code generator when the generated code is not placed on a dedicated file.

Before:

public function doFoo(Foo $foo) {}

After

public function doFoo(\Foo $foo) {}

This PR also takes into account the new PHP 5.4 callable data type when generating code.

Member

Ocramius commented Apr 8, 2013

@prolic ping? You used Zend\Code quite a bit - is this a huge BC break for you?

Contributor

prolic commented Apr 8, 2013

I am okay with these changes. This helps in more situations as I would expect troubles with it.

Owner

weierophinney commented Apr 12, 2013

Would it be possible to make this optional via a flag?

I ask, because our own CS indicates that parameter type hints should not be fully qualified; they should always resolve to current imports. The use case you're specifying here is highly specific -- it's for when classes are not in dedicated files, which, again, was not the original purpose, and goes against our own CS and recommendations.

That said, I can see the rationale for having this, but doing it as an optional behavior makes more sense to me.

Member

Ocramius commented Apr 12, 2013

I'll think of a way of making this optional. The fact is that this currently makes it impossible to have classes generated via generate() without a filegenerator too (for eval'd code)

Contributor

icywolfy commented Apr 16, 2013

Why would one need to use FQCN when multiple classes are in the same file/eval'd code?
So long as each section is prefixed with a non-global namespace; there's no issue with conflicting uses/namespaces.
And if you need to use the global namespace, then bracketed notation is better.

so

namespace Foo;
class A {}

namespace Foo;
use Foo\A as Bar;
class C extends Bar {}

namespace Foo;
interface B {}

namespace Foo\Bar\Baz;
use Foo\A as Baz;
use Foo\B as Bar;

class C extends Baz implements Bar {}

or if you need global support: (this isn't supported in the ClassGenerator)

namespace Foo {
class A{}
}
namespace {
use Foo\A as Bar;
class B extends Bar {}
}
namespace Foo {
use B as Bar;
class B extends Bar {}
}

But, I would personally see the above generated/used rather than giving users the option of using FQCN, since it's still much better to be using Use statements if only to promote better coding habits.

But that said, I have had no issues generating multiple classes into a single cached-file, or dynamic unclusion via eval() at work (hate using eval, but since the temporary classes don't live in the filesystem, can't really auto-load it)

Member

Ocramius commented Apr 16, 2013

@icywolfy this causes a number of problems atm.

Just as a simple example, take multiple imported classes with the same name.

Using the FQCN is not better from a CS perspective, but honestly, nobody should ever care about generated code CS.

Contributor

icywolfy commented Apr 16, 2013

@Ocramius Perhaps, but just don't see the issues with

    $x = new ClassGenerator('A');
    $x->setNamespaceName('Foo\Bot');
    $x->addUse('Foo\Bar\A', 'BarA');
    $x->addUse('Foo\Baz\A', 'BarB');
    $x->addUse('FooBar');
    $x->addMethod('doSomething', array(
      new ParameterGenerator('barA', 'BarA'),
      new ParameterGenerator('bazA', 'BazA'),
      new ParameterGenerator('botA', 'A'),
      new ParameterGenerator('subBotA', 'Sub\A'),
    ));
    $x->addMethod('somethingElse', array(
      new ParameterGenerator('barA', 'BarA'),
      new ParameterGenerator('api', 'FooBar\Service\ApiInterface $api'),
   ));

Or if you need to use a FQCN, explicitly prepend add the '' during generation;
(we just "use" our global and vendor prefixes namespaces)

Though I would love to have it propagate use-aliases set against methods; so that you can define the aliases you use in the method-body and have it resolve up to the class-level and be added to the generator (and throw exception if conflicting use-aliases are used)

But, we here heavily use the generated code for both sub-project generation, and for use of user and type management; User's are generated upon major changes, types are rebuilt/committed to code-base based on the config files.

We do manual changes for people to meet their desired behaviours and mark their classes as manually edited, and then treat them specially until such time that it's properly incorporated into the build/generation process. But since we are actually using\editing the generated code, it's nice to have it be readily readable.
Though fundamentally I'm against BC breaks now that ZF2 is relatively stable in the 2.1.x branch.

# <user>.php :
namespace <type>\<user>;
use <type>\User as BaseUser;

class User extends BaseUser {
   private $manualEdit = false;
   function getRole() { ...  return new Role\BaseRole()  }
   function hasRole($roleName) { return class_exists(Role\<$roleName>); }
...
}

namespace <type>\<user>\Role;
use <type>\<user> as BaseUser;
use <type>\TypeInterface;
class BaseRole implements TypeInterface { ... }

namespace <type>\<user>\Contact;
use ...
class Address extends Address\<country> { ... }

(we are operating without access to a database for several of the East-coast data-centres, and this was the solution that was ended up on, it's actually quite nice, if not a bit unconventional. Damned lawyers dictating how things work.)

Member

Ocramius commented Apr 17, 2013

That's way too complex for no real reason. Having FQCN simply removes all these problems at once. I'll see about the break - for now I had to subclass ALL generators to get rid of the problem, since the current API is unusable.

Member

ralphschindler commented Jun 6, 2013

@Ocramius after a cursory review, I'm ok with this, do you still feel like this should be merged?

Member

Ocramius commented Jun 6, 2013

@ralphschindler not sure since it's a bc break. Gimme some more time, otherwise feel free to close this and I'll redo it somehow.

Member

EvanDotPro commented Oct 15, 2013

@Ocramius ping? Let's either get this merged or refactored. 😄

Member

Ocramius commented Feb 4, 2014

This cannot land in 2.x - I'll keep my adaptation in my library and the issue open since it's a quite relevant "bug"

Owner

weierophinney commented Mar 3, 2014

Marking for 3.0.0

@weierophinney weierophinney added this to the 3.0.0 milestone Mar 3, 2014

@Ocramius Ocramius changed the title from Hotfix - FQCN in parameter type hints to [ZF3] FQCN in parameter type hints Apr 2, 2014

@Ocramius Ocramius added Code labels Apr 2, 2014

GeeH commented Jun 28, 2016

This issue has been moved from the zendframework repository as part of the bug migration program as outlined here - http://framework.zend.com/blog/2016-04-11-issue-closures.html
New issue can be found at: https://api.github.com/repos/zendframework/zendframework/issues/4149

@GeeH GeeH closed this Jun 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment