laminas / laminas-code

Extensions to the PHP Reflection API, static code scanning, and code generation
https://docs.laminas.dev/laminas-code/
BSD 3-Clause "New" or "Revised" License
1.85k stars 81 forks source link

[ZF3] FQCN in parameter type hints #23

Closed weierophinney closed 3 years ago

weierophinney commented 4 years ago

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


Original Issue: https://api.github.com/repos/zendframework/zendframework/issues/4149 User: @Ocramius Created On: 2013-03-30T11:59:09Z Updated At: 2014-09-28T00:34:50Z Body 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.


Comment

User: @Ocramius Created On: 2013-04-08T14:28:13Z Updated At: 2013-04-08T14:28:13Z Body @prolic ping? You used Zend\Code quite a bit - is this a huge BC break for you?


Comment

User: @prolic Created On: 2013-04-08T23:02:49Z Updated At: 2013-04-08T23:02:49Z Body I am okay with these changes. This helps in more situations as I would expect troubles with it.


Comment

User: @weierophinney Created On: 2013-04-12T16:36:04Z Updated At: 2013-04-12T16:36:04Z Body 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.


Comment

User: @Ocramius Created On: 2013-04-12T16:38:03Z Updated At: 2013-04-12T16:38:03Z Body 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)


Comment

User: @icywolfy Created On: 2013-04-16T16:57:20Z Updated At: 2013-04-16T16:57:20Z Body 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)


Comment

User: @Ocramius Created On: 2013-04-16T21:26:17Z Updated At: 2013-04-16T21:26:17Z Body @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.


Comment

User: @icywolfy Created On: 2013-04-16T23:05:33Z Updated At: 2013-04-16T23:05:33Z Body @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.)


Comment

User: @Ocramius Created On: 2013-04-17T07:42:23Z Updated At: 2013-04-17T07:42:23Z Body 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.


Comment

User: @ralphschindler Created On: 2013-06-06T22:48:45Z Updated At: 2013-06-06T22:48:45Z Body @Ocramius after a cursory review, I'm ok with this, do you still feel like this should be merged?


Comment

User: @Ocramius Created On: 2013-06-06T23:13:21Z Updated At: 2013-06-06T23:13:46Z Body @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.


Comment

User: @EvanDotPro Created On: 2013-10-15T03:05:44Z Updated At: 2013-10-15T03:05:44Z Body @Ocramius ping? Let's either get this merged or refactored. :smile:


Comment

User: @Ocramius Created On: 2014-02-04T12:05:50Z Updated At: 2014-02-04T12:05:50Z Body 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"


Comment

User: @weierophinney Created On: 2014-03-03T16:31:55Z Updated At: 2014-03-03T16:31:55Z Body Marking for 3.0.0



Originally posted by @GeeH at https://github.com/zendframework/zend-code/issues/76

Ocramius commented 3 years ago

Closing here: the FQCN used when generating method/property signatures are (by design) to be prefixed with \\, as otherwise it becomes extremely tricky (and with very little ROI) to generate valid code.