Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

allowing more event identifiers in AbstractController #248

Closed

Conversation

kokspflanze
Copy link
Contributor

hi =)

my main problem was like zendframework/zendframework#6553, but with a other namespace of my module.

i had 2 modules which has following names Foobar\Baz and Foobar\Bar, with the old logic it was not possible to know which controller module was called. It was everytime just Foobar in the event identifiers.

With the change it will generate a list with different namespaces so it will generate Foobar, Foobar\Baz or Foobar\Bar.

@froschdesign froschdesign added this to the 3.2.0 milestone Jul 20, 2017
@Xerkus
Copy link
Member

Xerkus commented Nov 22, 2017

I'm not sure this is a good idea tbh. You want to declare extremely common identifiers, and identifiers are shared across ALL of the event managers using same shared event manager, they are not limited to controllers only.

I believe proper approach would be to provide marker interfaces for all the controllers in the module:
class FooController extends AbstractActionController implements \Foobar\Baz\Controller\MarkerInterface
Bonus here is if you want to override controller, all you need is to implement marker interface to keep shared listeners working as expected.

@kokspflanze
Copy link
Contributor Author

@Xerkus a markerinterface works sure, but only for own modules, if you have external dependencies, you have to overwrite all controllers to add a markerinterface.

];

$offset = 0;
for ($i = 0; $i < substr_count($className, '\\'); $i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think substr_count can be assigned to variable for micro-opt

@Xerkus
Copy link
Member

Xerkus commented Apr 22, 2018

I still oppose addition of namespaces at all levels as identifiers.
Considering practice to put all the controllers into own */Controller ns, I would support addition of full controller namespace as identifier: substr($className, 0, strrpos($className, '\\'))
That would give you per-module identifier and it would minimize unintended identifier collisions.

];

$offset = 0;
$substrCont = substr_count($className, '\\');
Copy link
Contributor

Choose a reason for hiding this comment

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

s/$substrCont/$substrCount

@Xerkus
Copy link
Member

Xerkus commented Feb 17, 2019

Since no more input was given I decided to proceed with #282
I am going to close this PR now. If you feel #282 was not enough to solve the use case, feel free to reopen.

@Xerkus Xerkus closed this Feb 17, 2019
@kokspflanze
Copy link
Contributor Author

@Xerkus for me not realy enough, i wait now over 1year.
Which input is missing? What you need?
#282 is not realy enough, its for me to support 1 more case but not all.

@kokspflanze
Copy link
Contributor Author

@Xerkus i can not reopen...

@Xerkus
Copy link
Member

Xerkus commented Feb 17, 2019

@kokspflanze can you please describe use case that requires more generic identifiers?

@kokspflanze
Copy link
Contributor Author

@Xerkus

if you have the following classes

  • TeamName\Project\Admin\Controller\FoobarController
  • TeamName\Project\Admin\SubModule\Controller\FoobarController

if you enter the TeamName\Project\Admin\SubModule\Controller\FoobarController Controller, it should also trigger the TeamName\Project\Admin event.
So it would be possible to set the layout, just based on TeamName\Project\Admin and not for each module.
There are also much more parts possible not just change layout, also render or error handling based on the module name would be possible.

@tux-rampage
Copy link
Contributor

IMHO this adds too much magic to controllers. MVCs event system is already pretty complex and hard to debug and this would add additional complexity.

It might be even harmful for existing projects out there, that rely on the current behavior - therefore it seems to introduce a BC break.

Just my 2¢

@Xerkus
Copy link
Member

Xerkus commented Feb 19, 2019

Let me explain my hesitation to accept this PR.

Problem with expanding the scope is that identifiers are virtually global with a single shared event manager used throughout the application. Any other EventManager based event driven implementation under TeamName\Project\Admin namespace, following same logic for assigning identifiers, would trigger shared event listener for changing layout. Event based hooks in model logic? Sure. Something that have nothing to do with MVC at all, just happens to use events? Sure.
The only distinguishing factor would be a very generic event name. It is only dispatch for controllers, as provided by ZF itself, but userland code can freely add more for their needs and I can easily see it used in userland code in different context.

Events in zend-mvc are already presenting a pretty steep learning curve. Listeners interacting wrong is not easy to debug already and for someone who is not deeply familiar with framework this side-effect could be pretty devastating.

In short, usage of namespace hierarchy as identifiers is a misuse of the feature. To quote the @weierophinney:

Event identifiers should generally be actual class or interface names. If we start naming them after the namespace hierarchy, we start getting into "magic string" territory, and there's a huge potential for conflicts.

So, after some consideration, I decided not to proceed in this direction. Sorry.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants