I'm going to argue this isn't a B/C break because plugins like that essentially relied on the behavior of JDocument renderer classes not being autoloaded hence they could overload them. In theory they still can by manipulating the autoloader, they just must do so before the renderer classes get loaded (so an onAfterInitialise event would work fine since JDocument isn't initialized until after that) and they must do so with the new classes which are given priority to get the expected behavior. The classes had to be renamed to both fit autoloader convention and to prevent naming collisions across formats (for example if both the JSON and HTML formats had a message renderer with different behaviors, a JDocumentRendererMessage class couldn't be used by both).

The BC break is more because the expectation of the autoload order changed. Since we don't have a IoC container to overload the classes properly, the changed functionality in JDocument::loadRenderer requires additional workaround for >3.5.

Following semver, either JDocument::loadRenderer is a private API (and 3PD must deal with it) or there must be some way to deal with BC -- even adding a boolean parameter to loadRenderer that defaulted to the old way and logged a deprecation warning while but passing in the opposite value would trigger the new way with no warning.

A parameter wouldn't cut it. It would have to take the non-autoloaded
class first which then defeats the point of internally restructuring the
classes. Even if it were a private function plugins would still be able to
manipulate the old behavior because everything was manually included
before. My personal interpretation is that extensions shouldn't be relying
on the fact some classes in the core API are manually loaded to be able to
overload them and doing so is an action at your own risk and not covered
under a B/C policy. 3.5.0 broke I believe it was redCORE's overloading of
JFormField (an autoloaded class) adding and implementing a new method, by
comparison you could argue this too was a B/C break.

On Wednesday, April 6, 2016, Christopher Roussel notifications@github.com
wrote:

The BC break is more because the expectation of the autoload order
changed. Since we don't have a IoC container to overload the classes
properly, the changed functionality in JDocument::loadRenderer requires
additional workaround for >3.5.

Following semver, either JDocument::loadRenderer is a private API (and
3PD must deal with it) or there must be some way to deal with BC -- even
adding a boolean parameter to loadRenderer that defaulted to the old way
and logged a deprecation warning while but passing in the opposite value
would trigger the new way with no warning.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9764 (comment)

The parameter could cut it by changing the if statements to if (!class_exists($class) || $useDeprecatedLoading) in https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/document/document.php#L999 and L1004.

Your interpretation of BC policies doesn't mesh with that of semver -- which is fine if Joomla doesn't follow semver. According to semver, either JDocument::loadRenderer() is public API or private API. If it's public, then this was a BC break and needs to be fixed. If it's private API, then it's not a BC break. I'm asking for clarification here: is the method public or private?

The method is declared public but what about the API has changed? It still
returns a JDocumentRenderer instance however its load order (which is an
internal method behavior) changed. Doing so broke an undocumented
(unsupported?) behavior of enabling third parties to overload classes and
requires updates to overload a new class name. So is it Joomla's stance
that overloading of core classes in this manner is a part of its B/C
policy? I say no because overloading in this manner equates to a core hack
and an undocumented change of the expected API. If it's decided that this
is a B/C break, then the only option going forward is to freeze the
existing API of all classes since a change in an internal behavior or the
addition of new methods can cause breaks in extensions using this behavior.

On Wednesday, April 6, 2016, Christopher Roussel notifications@github.com
wrote:

The parameter could cut it by changing the if statements to if
(!class_exists($class) || $useDeprecatedLoading) in
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/document/document.php#L999
and L1004.

Your interpretation of BC policies doesn't mesh with that of semver --
which is fine if Joomla doesn't follow semver. According to semver, either
JDocument::loadRenderer() is public API or private API. If it's public,
then this was a BC break and needs to be fixed. If it's private API, then
it's not a BC break. I'm asking for clarification here: is the method
public or private?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9764 (comment)

Public API vs private API has nothing to do with method visibility but on consumption.

The API has changed because calling the same method with the same parameters in 3.4 returned a different result. If there were a unit test on that method, it would have had to be rewritten because the logic changes for 3.5 changed the logic (as you yourself said). That is a potential break with BC.

For methods and classes that are external and fit for public dev consumption, semver requires an increase of the major version. However, semver also requires a minor release that provides the same response as before with deprecation warnings so developers can update their code before the next major release. Which is why I suggested the parameter to allow the transition.

For methods and classes that are internal (i.e private), semver requires nothing.

Like I said, if it's the stance that Joomla supports class overloading
(which is how this all worked before) then this PR and several other PRs
which have caused classes to be loaded in different orders or autoloaded
should be reverted in full (this includes changes to the application
classes, JForm, JHtml, and JInstaller at a minimum). I don't agree that
support of class overloading is a part of Joomla's API or B/C policy. The
load order doesn't mean a thing here and the old logic is still supported.
The problem boils down to the fact that extensions are relying on core
hacks to accomplish a change in behavior.

On Wednesday, April 6, 2016, Christopher Roussel notifications@github.com
wrote:

Public API vs private API has nothing to do with method visibility but on
consumption.

The API has changed because calling the same method with the same
parameters in 3.4 returned a different result. If there were a unit test on
that method, it would have had to be rewritten because the logic changes
for 3.5 changed the logic (as you yourself said). That is a potential break
with BC.

For methods and classes that are external and fit for public dev
consumption, semver requires an increase of the major version. However,
semver also requires a minor release that provides the same response as
before with deprecation warnings so developers can update their code before
the next major release. Which is why I suggested the parameter to allow the
transition.

For methods and classes that are internal (i.e private), semver requires
nothing.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9764 (comment)

Class overloading is necessary unless there's some kind of Dependency Injection which allows loose coupling and a more natural overloading. Since Joomla does allow overloading a core class by injecting it via JLoader::register() before its own core classes are autoloaded (which should be done long before the application initializes and via composer's autoload.php), it sounds like Joomla does support overloading (which is preferable to hacking the core files directly).

Then my legitimate suggestion if that is the project's stance is to revert
the JDocument PR in full and deprecate the autoloader in favor of 1.7 style
class loading. That's not me trying to troll either. If Joomla is going
to take the stance that overloading is supported then the full API needs to
support this, not one class chain only.

On Wednesday, April 6, 2016, Christopher Roussel notifications@github.com
wrote:

Class overloading is necessary unless there's some kind of Dependency
Injection which allows loose coupling and a more natural overloading. Since
Joomla does allow overloading a core class by injecting it via
JLoader::register() before its own core classes are autoloaded (which
should be done long before the application initializes and via composer's
autoload.php), it sounds like Joomla does support overloading (which is
preferable to hacking the core files directly).

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9764 (comment)

This will be more productive if it is moved to the mailing list

On 6 April 2016 at 20:31, Michael Babker notifications@github.com wrote:

Then my legitimate suggestion if that is the project's stance is to revert
the JDocument PR in full and deprecate the autoloader in favor of 1.7 style
class loading. That's not me trying to troll either. If Joomla is going
to take the stance that overloading is supported then the full API needs to
support this, not one class chain only.

On Wednesday, April 6, 2016, Christopher Roussel <notifications@github.com

wrote:

Class overloading is necessary unless there's some kind of Dependency
Injection which allows loose coupling and a more natural overloading.
Since
Joomla does allow overloading a core class by injecting it via
JLoader::register() before its own core classes are autoloaded (which
should be done long before the application initializes and via composer's
autoload.php), it sounds like Joomla does support overloading (which is
preferable to hacking the core files directly).

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
<#9764 (comment)

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#9764 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

No need to revert all changes. Just follow semantic versioning in deprecating APIs.

We are not going to be changing this. We are not changing the behaviour of JDocument::loadRenderer. The JLoader was never designed to override core classes and this has always been strongly discouraged (although we obviously have no way of preventing this) - it was designed for allowing 3rd parties to add their own extensions/libraries into the autoloader where appropriate.

We won't be changing this. The plugin you mentioned has adapted and I consider this case closed

I'm still not convinced there's a change in the public facing API. The loadRenderer() method returns a JDocumentRenderer subclass pre- and post- 3.5. What changed is the internals of that method to give precedence to a different load order which affects overloading of the core document renderers. If we're calling a change in a method's internals a B/C break then I'm not sure what you CAN change that wouldn't constitute a B/C break (bug fixes change a behavior too).

My issue is that by making changes which favor core overloading, we are not addressing the real issues that need to be and we're still encouraging developers to implement features that hack core. Few APIs that have factory methods to retrieve an object allow you to customize anything (so your custom objects have to be in the Joomla core class space (J prefix) for custom objects not colliding with core classes or require you to overload core classes). The overloading is dangerous to support because it causes unexpected breaks of extensions (and inherently user's sites) by making changes to the core API (reference this issue and the above noted change with JFormField). So in all reality the only way to properly support overloading is to freeze the API on existing classes and methods; new code has to be added to something that doesn't already exist, and that spells major trouble for an organized or structured API. Joomla core needs to be moving in a direction that does NOT favor core hacking/overloading but still allows customization of behaviors, and doing a better job at it (I'll pull up an old concept I had that directly relates to JDocument, see https://github.com/mbabker/joomla-next/blob/master/libraries/core/Document/DocumentFactory.php).

Yes, the JDocument change causes the load order on objects to change. Yes, it causes a new subclass to be loaded. Yes, it means that plugins that overloaded renderers pre-3.5 don't overload those renderers anymore. I truly believe that last statement should not affect core restructuring because we should not be encouraging core hacks/overloads to add features. And if we're of the stance that core needs to support overloading to add features then we need to take steps that reverse changes which complicate overloading or have caused different class objects to be loaded than in previous releases.

It's not about whether a particular plugin was updated but rather that Joomla follows semantic versioning so that dependent packages aren't broken unexpectedly. The logic change from JDocument::loadRenderer() returning JDocumentRendererModules to returning JDocumentRendererHtmlModules broke at least one plugin during a minor version release.

The issue at hand isn't about overloading via JLoader::register() which can be fixed with core libraries being loaded before plugins can inject new classes. Rather, the issue was whether JDocument::loadRenderer() is a public API (whose logic should be changed only during major releases) or a private API (which doesn't have that restriction). Just because a public-facing API doesn't change its return type doesn't mean that BC isn't broken. Changing a public increment function to increment by 2 instead of 1 still returns an integer but it does change expected output.

It seems that there is resistance to changing this to sustain BC, so perhaps a new request should be that Joomla identifies what its public API is for purposes of semver protection. JDocument::loadRenderer() sounds like it should be internal/private API.

If we're going to get into that particular of details, then the API would have to document that X argument under Y condition would produce Z result since loadRenderer() is in essence a factory method which can instantiate numerous objects. In the absence of that, the generic "I call loadRenderer() and get a JDocumentRenderer instance" test/statement holds true to the method's behavior pre- and post- change. The class implementing that contract did change, the behaviors of the returned classes didn't.

The application bootstrap calls JLoader::setup() which instantiates the autoloader and adds the autoload paths for the J class prefix so that classes are loaded from (in order) libraries/cms, libraries/joomla, and libraries/legacy. What this does NOT do is load the classes existence into memory. I can't say I know the internals of our autoloader or PHP autoloading in general too well, but I believe the way we're set up until you do something that explicitly requires a class it hasn't been loaded into place. That's why even after the lookup prefixes are set during the bootup process you can still call JLoader::register() and change a lookup path for a class until its first use (either by instantiating it, calling class_exists(), or something else that would cause a class to be loaded into memory).

The only way to be B/C with this is to revert #8905. That PR was necessary because you can not add custom lookup paths for JDocument objects and trying to register custom objects to the API via the class autoloader would just cause naming and/or function collisions. At the time that code was written, we were looking at a way to introduce support for Google AMP to the Joomla core and found that there was little practical way to add a JDocumentAmp subclass and be able to use plugins to support this. After the initial change to JDocument::getInstance() to better support the autoloader, the naming format for JDocumentRenderer subclasses would have caused another issue because we would not have been able to register AMP renderers to the autoloader without getting into function collisions with the core HTML renderers, so we needed to be able to add "namespaced" renderers based on the format. It came at the expense of breaking the old load order processing.

Yes, your parameter toggle method might restore the old behavior, but honestly after all of the digging that went into creating that PR, I can't think of a manner in which it would work without introducing issues. Plugins that overloaded JDocumentRendererModules for example were pushing it into the autoloader and making that the preferred renderer for when loadRenderer('modules') was called regardless of format and if core ever supported rendering modules in a non-HTML format that would cause a new round of issues. I would strongly suggest that the PR which broke B/C fixed more bugs than introduced regressions after going through that exercise.

As per semver, API methods should maintain the same output with the same input within a major version. It is that particular of details. This issue implies that such functionality did break. It appears that Joomla devs consider that method to be internal and thus not protected by semver.

class_exists calls __autoload() by default. Apparently, JLoader::setup() does not follow this convention like composer packages do. Since JDocumentRendererModules and JDocumentRendererHtmlModules (for example) can be overloaded, it seems that their references are not autoloaded with JLoader::setup() (which follows PHP convention of using spl_autoload_register()). "Autoloading" a class doesn't mean injecting it into memory but reserving its reference for later usage. Using the class or checking it via class_exists() would bring the class into memory.

I am not arguing that the PR which broke BC did not have usefulness or even better functionality. I am also not arguing the merits of autoloading vs the J1.7 way of loading classes. I am arguing that 3.5.0 broke expected API behavior. Either the class/method should be documented as internal/private, there should be an immediate 4.0.0 release, or a patch/minor release should be made restoring backwards compatibility. That's semver which Joomla follows. My PR was an attempt to do the last of those. It shouldn't cause harm because it doesn't attempt to load the core class that has been overloaded but rather swaps back the preference for the generic JDocumentRendererModules over the specific JDocumentRenderer{Type}Modules as it performed in 3.0 - 3.4. As I've said a few times now, it seems the Joomla core devs response is the first, which is a perfectly acceptable answer.

As a general rule, if you need to change in a unit test's provided input or output assertion (like in 8905, you're likely breaking BC for that method. If the method is supposed to be part of the public API, semver dictates the major version must be increased. If it's not part of the public API, then nothing special must be done.

I think the issue I have is that actually it's your use of the JLoader::register method. Let me reiterate this is not designed to override core classes. This is designed to supplied core classes which aren't autoloaded or extension classes (e.g. in modules how we load the com_content modules). It is NOT designed to allow you to override core classes in a system plugin. And this has been repeated so many times in various places. If you are doing this you are using the API incorrectly and thus live by the consequences.

Moving on the JDocument::loadRenderer method is documented in the doc block as loading a JDocumentRenderer renderer object which it does fully following b/c if you input a given type (and even if you specify a custom type). There is no documented way of adding custom renderers so as far as I'm concerned it's a public method and it follows b/c.

FWIW of course what I say and what you say are both completely valid statements and are two different ways of looking at the same event and honestly it's one of these grey lines. I think in this case for the low impact but high outcome (getting towards the point of autoloading everything) - I think this is fine.

FWIW the unit tests we have are a load of crap. If we were unit testing the method properly we should validate against the return typehint in the doc block rather than an actual class that comes out (in which case we'd have kept b/c). Our unit tests have needed a rewrite for a long long time but it's one of those things that is a nightmare with the current architecture and there's always "one more thing" ahead of it in the list....

I commented the one downfall I saw on your PR. To be frank though instead of a parameter you're better off either changing the load order in the code so that core tries to autoload the old objects (which has the issues I noted above), then tries to manual include the old style classes, then tries the new classes or just remove the loading for the new classes completely.

The only way overloading is done "right" is if plugins are overloading during onAfterRoute so that (hopefully) the right format is already known by the application and said plugins are conditionally loading for their desired formats because of the issues pointed out. This is also the last event trigger before the application instantiates JDocument. The plugin you linked uses onAfterInitialise, which while it works, has the downfall of being unaware of the route format so it's loading the modules renderer to all formats (generally not an issue because core doesn't have renderers for anything but full HTML pages and RSS/Atom and none that even share a name between formats, but should the day ever come that Joomla actually properly supports something other than HTML...).

Also as per your link to semver Use your best judgment. If you have a huge audience that will be drastically impacted by changing the behavior back to what the public API intended - I'd argue that one plugin in the thousands of extensions that exist is not a huge audience - especially as that plugin has fixed the issue

The usage of JLoader::register has no effect on JDocument::loadRenderer returning JDocumentRendererHtmlModules instead of JDocumentRendererModules when calling it with the parameter of 'html' since 3.5.0. This doesn't matter if the class is overloaded at onAfterInitialise or onAfterRoute.

Nobody from Joomla is arguing that this wasn't intentional. Nobody is arguing that overloading of core classes using JLoader::register isn't bad/harmful/warranty-voiding/immoral/fattening/whatever. Nobody is arguing that the method signature or return type for JDocument::loadRenderer has changed.

What is being asked here is whether JDocument::loadRenderer is a public API method (which, consequently, is bound to semver "freezing"). If it is, then providing it a set of parameters should return the exact same result throughout minor releases. JDocumentRendererModules !== JDocumentRendererHtmlModules even if JDocumentRendererHtmlModules extends JDocumentRendererModules {} (or JDocumentRendererModules extends JDocumentRendererHtmlModules {}).

This breaks functionality of downstream code in at least one extension that consume the Joomla API. There is no grey line here:expected functionality supposedly protected by semver was broken. Whether the functionality should be fixed may be a grey area because there may be only a small audience which makes this a much lower priority and essentially not worth addressing. Whitewashing the issue that Joomla developers have at least once broken expected functionality of an API method or arguing that this changed functionality does not somehow break BC is a rarely a good choice.

There's no way to extend the core class and overload just the different functionality (which would have made redCORE's issue non-existent) and replace the bound implementation in the IoC container (as a loosely coupled framework would do). So, we're left with either hacking the core directly (a horrible solution), throwing away the extension until Joomla core makes it possible to perform this hotloading functionality, exploiting JLoader::register to overload the class, or giving up and using a DI/IoC capable framework (Laravel is very popular these days) which has that functionality.

That you're saying the unit tests -- which are meant to ensure there are no regressions with the public API -- is crap doesn't mean that the changed functionality did not break BC or that Joomla shouldn't be trying to maintain BC. You can be happy that PHP7 supports typehinting return values which will make the automagic parsing of docblocks for expected return type for testing obsolete.

If it is, then providing it a set of parameters should return the exact same result throughout minor releases. JDocumentRendererModules !== JDocumentRendererHtmlModules even if JDocumentRendererHtmlModules extends JDocumentRendererModules {} (or JDocumentRendererModules extends JDocumentRendererHtmlModules {})

Wait, what? If ($document->loadRenderer('modules') instanceof JDocumentRendererModules) passes as true both before and after a change, how or why should it matter that either JDocumentRendererModules is now a subclass of a new JDocumentRendererHtmlModules (as long as the class inheritance from JDocumentRenderer isn't broken) or that the API returned an instance of JDocumentRendererHtmlModules which extends JDocumentRendererModules? Unless I'm misreading something here...

If ($document->loadRenderer('modules') instanceof JDocumentRendererModules) returned true in 3.5.0, this PR wouldn't exist.

if it did, then

$renderer = $document->loadRenderer('modules');
$inheritance = ($renderer instanceof JDocumentRendererModules); // true
$child_equality = (get_class($renderer) === 'JDocumentRendererModules'); // true
$parent_equality = (get_class($renderer) === 'JDocumentRendererHtmlModules'); // false

in 30 - 3.4 (get_class($document->loadRenderer('modules')) === 'JDocumentRendererModules')) would be true. That's why NNBootstrapParams overloaded that class only. In 3.5, that boolean is false because it now returns JDocumentRendererHtmlModules.

As I've pointed out, the "load of crap" tests were even changed to assert a different returned class (see the diff for 8905).

If ($document->loadRenderer('modules') instanceof JDocumentRendererModules) returned true in 3.5.0, this PR wouldn't exist.

Ya I get that but the way I read what you said at first it was almost as if you were saying that even if a JDocumentRendererHtmlModules instance was returned that passed that check (so it would have to subclass JDocumentRendererModules) it would still be a B/C break and that was throwing me for a loop.

What is being asked here is whether JDocument::loadRenderer is a public API method (which, consequently, is bound to semver "freezing").

Joomla's definition of "public API" is any declared public or protected method or variable (I assume constants go in there too but honestly I don't remember the dev strategy text and I'm just being lazy now and not looking). By the contract of the API (the documented parameters and the expected return), B/C did not break as you still receive a JDocumentRenderer instance. An implementation detail of the method did change which results in a different class being returned from previous versions which performs the same functions. From that aspect, yes a B/C break occurred. I think everyone here has already agreed on that.

That you're saying the unit tests -- which are meant to ensure there are no regressions with the public API -- is crap doesn't mean that the changed functionality did not break BC or that Joomla shouldn't be trying to maintain BC.

I've been working a lot with the unit tests, as has George. Both of us will tell you first hand the unit tests for the CMS are a borderline joke. Unless it's acceptable that there are 566 test cases covering the constructor for JApplicationCms of which maybe 50 come from a test case directly testing that or one of its subclasses.

There's no way to extend the core class and overload just the different functionality (which would have made redCORE's issue non-existent) and replace the bound implementation in the IoC container (as a loosely coupled framework would do).

The problem with redCORE's issue and the one here isn't necessarily in needing to change the behavior of a single method. With the JFormField change, because it's a parent class to so many other objects you can't just easily subclass it to change one behavior and have all descendants accept that change and IoC wouldn't help with that. For the JDocument stuff, lacking implementation of Joomla's DI Container in the CMS or any goals to even attempt it, the next "best" available option to subclass core objects would be a way to specify a class prefix instead of hardcoding conventions into factory methods like JDocument::loadRenderer() or go the way JForm, JHtml, or even the MVC layer are implemented and use arrays of lookup paths to find objects if not found by the autoloader first. At least in this way you get some form of being able to use custom objects without mandating a need for overloading.

If you really want to change it to support the old behavior, this will do it. I still say this is wrong as it restores buggy and borderline invalid logic as the first priority to be loaded but it satisfies the semantics for arguing B/C:

diff --git a/libraries/joomla/document/document.php b/libraries/joomla/document/document.php
index fe4af66..66a0f6a 100644
--- a/libraries/joomla/document/document.php
+++ b/libraries/joomla/document/document.php
@@ -993,36 +993,40 @@ class JDocument
     */
    public function loadRenderer($type)
    {
-       // New class name format adds the format type to the class name
-       $class = 'JDocumentRenderer' . ucfirst($this->getType()) . ucfirst($type);
+       // "Legacy" class name structure - deprecated, use "namespaced" renderers with the format in the class name
+       $class = 'JDocumentRenderer' . $type;

-       if (!class_exists($class))
+       if (class_exists($class))
        {
-           // "Legacy" class name structure
-           $class = 'JDocumentRenderer' . $type;
-
-           if (!class_exists($class))
-           {
-               // @deprecated 4.0 - Non-autoloadable class support is deprecated, only log a message though if a file is found
-               $path = __DIR__ . '/' . $this->getType() . '/renderer/' . $type . '.php';
+           return new $class($this);
+       }

-               if (!file_exists($path))
-               {
-                   throw new RuntimeException('Unable to load renderer class', 500);
-               }
+       // @deprecated 4.0 - Non-autoloadable class support is deprecated, only log a message though if a file is found
+       $path = __DIR__ . '/' . $this->getType() . '/renderer/' . $type . '.php';

-               JLog::add('Non-autoloadable JDocumentRenderer subclasses are deprecated, support will be removed in 4.0.', JLog::WARNING, 'deprecated');
-               require_once $path;
+       if (file_exists($path))
+       {
+           JLog::add('Non-autoloadable JDocumentRenderer subclasses are deprecated, support will be removed in 4.0.', JLog::WARNING, 'deprecated');

-               // If the class still doesn't exist after including the path, we've got issues
-               if (!class_exists($class))
-               {
-                   throw new RuntimeException('Unable to load renderer class', 500);
-               }
+           require_once $path;
+   
+           // If the class still doesn't exist after including the path, we've got issues
+           if (class_exists($class))
+           {
+               return new $class($this);
            }
        }

-       return new $class($this);
+       // New class name format adds the format type to the class name
+       $class = 'JDocumentRenderer' . ucfirst($this->getType()) . ucfirst($type);
+
+       if (class_exists($class))
+       {
+           return new $class($this);
+       }
+
+       // All supported lookups are exhausted, we did our best
+       throw new RuntimeException('Unable to load renderer class', 500);
    }

    /**

EDIT: Just note applying this diff to any release now introduces yet another B/C break as precedence once again goes to the old class names so at least in the case of nnbootstrapparams yet again the version conditional will have to be adjusted.

API compatibility !== backwards compatibility. The former is more about interfaces, implementations, method signatures, and return types. The latter is more about getting the same response from the same method. If JDocument::loadRenderer were a RESTful API endpoint (e.g. GET /jdocument/renderer), one would expect it to return the same value for the entire lifecycle of API v1, regardless of what changes occur to the underlying codebase.

Also, I don't know why anyone thinks a response of "well if you want the old code, here it is and don't ask us for help" is a good one. The new logic is fine but it needs to return the same response during the entire 3.x lifecycle. That means new logic should be opt-in until the next major release. Old logic can be deprecated as long as it still returns the same value. Deprecation warnings can be added.

What Joomla conveys in its versioning is important -- which is why there was the move to use semver from its old methodology. I've been around Joomla since Mambo 4.5.1, and it's starting to follow wider PHP practices (composer, namespacing, semver, unit testing, etc) which is very welcome. Telling developers that BC breaks are acceptable or arguing that isn't a BC break because the API contract hasn't changed is a step backwards.

I can't make that method opt in on a new API in the way that it is used in
core 99% of the time (the jdoc tags get converted to PHP data that loads
the relevant renderers). So it's either the old load order takes
precedence and only if anything CANNOT be found with that THEN the new way
is attempted. That's what that diff does. That is the ONLY way to give
any resemblance of B/C short of fully reverting that PR. And even then
it's not 100% B/C because the old class is not a direct subclass of
JDocumentRenderer but now subclasses the new classes. So I'm at the point
where it's either take it or leave it. I've argued why reverting is just
as bad as leaving things as is. And frankly I'll stop refactoring APIs now
since any change is a B/C break.

On Thursday, April 7, 2016, Christopher Roussel notifications@github.com
wrote:

API compatibility !== backwards compatibility. The former is more about
interfaces, implementations, method signatures, and return types. The
latter is more about getting the same response from the same method. If
JDocument::loadRenderer were a RESTful API endpoint (e.g. GET
/jdocument/renderer), one would expect it to return the same value for
the entire lifecycle of API v1, regardless of what changes occur to the
underlying codebase.

Also, I don't know why anyone thinks a response of "well if you want the
old code, here it is and don't ask us for help" is a good one. The new
logic is fine but it needs to return the same response during the entire
3.x lifecycle. That means new logic should be opt-in until the next major
release. Old logic can be deprecated as long as it still returns the same
value. Deprecation warnings can be added.

What Joomla conveys in its versioning is important -- which is why there
was the move to use semver from its old methodology. I've been around
Joomla since Mambo 4.5.1, and it's starting to follow wider PHP practices
(composer, namespacing, semver, unit testing, etc) which is very welcome.
Telling developers that BC breaks are acceptable or arguing that isn't a BC
break because the API contract hasn't changed is a step backwards.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9764 (comment)

The logic is using strings, checking if the class exists, and if it does't, falling back to another class string. Both new and old do that. The main difference is that the new first checks if the specific media type is available via autoloading. Then the new checks to see if the generic class is available via autoloading, which is what the new does first. Then both manually require the media-specific file (or throw an exception).

The way to opt-in to the new logic is to add a flag which defaults to returning the generic class.

The flag has no value unless you code it into the logic that parses jdoc
tags into PHP data. loadRenderer generally doesn't get called outside
JDocument (there are a couple exceptions). That's why my diff is
essentially the only way to do it without adding a lot more logic to the
API. Basically the new class names have to be the fallback after ALL
attempts to load the old way.

On Thursday, April 7, 2016, Christopher Roussel notifications@github.com
wrote:

The logic is using strings, checking if the class exists, and if it
does't, falling back to another class string. Both new and old do that. The
main difference is that the new first checks if the specific media type is
available via autoloading. Then the new checks to see if the generic class
is available via autoloading, which is what the new does first. Then both
manually require the media-specific file (or throw an exception).

The way to opt-in to the new logic is to add a flag which defaults to
returning the generic class.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9764 (comment)