Finally got the time to review this. I still think it's wrong. getLayoutPaths() shouldn't know anything about the renderer so passing it as parameter is not ok.

It's better to call JLayoutFile inside getLayoutPaths() than tying it to a function in the renderer because that will force to change the renderer interface to at least cover it.

So I think that based in our requirements the best solution is to add a direct call to a dummy renderer inside getLayoutPaths():

	/**
	 * Allow to override renderer include paths in child fields
	 *
	 * @return  array
	 *
	 * @since   3.5
	 */
	protected function getLayoutPaths()
	{
                $renderer = new JLayoutFile;

		return $renderer->getDefaultIncludePaths();
	}

JLayoutFile is already used in the base getRenderer() so it's ok to also add a explicit renference there to get the paths from it. That way child classes can do something like:

	/**
	 * Allow to override renderer include paths in child fields
	 *
	 * @return  array
	 *
	 * @since   3.5
	 */
	protected function getLayoutPaths()
	{
                $paths = parent::getLayoutPaths();

                array_unshift(JPATH_ROOT . '/modules/mod_mymodule/layout/', $paths);

		return $paths;
	}

and it will always return what is expected in a B/C way. I think that's the best we can do until we use containers.

Hi @phproberto.

Thanks for the review. You probably realise this is my first contribution to Joomla core, so it's really helpful to get feedback like this as it will help me learn for future contributions. (as a user, it's also reassuring to see this process, because it gives a lot of confidence in the final product)

I am mildly worried that having getLayoutPaths() instantiate its own renderer will mean that it isn't guaranteed to return the same default paths as the actual renderer, which is why I elected to pass the object. But it works fine in my use-case with a dummy object and I think probably most other cases too, so I agree it's not something to worry about too much.

I was going to start asking you further questions to debate it, but on reflection I think I'm content to agree with you, and I've updated the PR per your suggested code.

Hi @phproberto.

I know I've left this for a long time, but I wonder whether you'd be able to review it again.

I made the changes that you suggested after the previous review, so I was hoping you'd have taken a look. But that was a couple of months ago now.

If you do have a chance to take a look, I'd really appreciate it. Thank you :)

@phproberto Could you follow up on this? Thank you.

If this Issue get no Response, it will be closed at 22th September 2018.

Set to "closed" on behalf of @franz-wohlkoenig by The JTracker Application at issues.joomla.org/joomla-cms/13018

This has been closed due to lack of response to the requests above – it can always be reopened.

This has been closed due to lack of response to the requests above – it can always be reopened.

Hey @Spudley I hope you still can read this. I just realised this has been blocked by me for 2 years which is the worst thing I could do with a new contributor.

I'm really sorry. I have refreshed this PR keeping you as author in:

#22645

And added some test to cover FormField::getLayoutPaths()

I know that's not enough but I hope it helps.

Sorry again mate!

And thanks @roland-d for trying to take care of my blocked stuff!

@phproberto -- no worries ? Thank you for picking it up again.