Fixes #39 Fixes #40 remove laminas-loader dependency

[samsonasik]

Q	A
New Feature	Yes
BC Break	No

Description

Re-create and modify zendframework/zend-mail#186 by @acelaya

Fixes #39
Fixes #40

Original PR description:

This removes the dependency on the zendframework/zend-load package, as suggested in #185

The HeaderLoader class has been removed and replaced by a simple class map in the Headers class.

However, I have also removed the getPluginClassLoader and setPluginClassLoader methods.

Actual changes

• Bumps minimum supported PHP version to 7.1
• Introduces Laminas\Mail\Header\HeaderLocatorInterface, defining how to map header names to classes.
• Introduces Laminas\Mail\Header\HeaderLocator as a default implementation of that interface.
• Modifies Laminas\Mail\Headers as follows:
  • Marks setPluginClassLoader() and getPluginClassLoader() as deprecated.
  • Adds a $headerLocator property and accompanying setHeaderLocator() and getHeaderLocator() methods.
  • Adds a private method, resolveHeaderClass() that:
    • if a non-null $pluginClassLoader property is available, uses that to locate the header class.
    • otherwise, uses the return value of getHeaderLocator() to locate the header class.

This approach provides a forwards-compatible approach to removing laminas-loader in version 3.0.

[samsonasik]

travis build green 🎉

[glensc]

@samsonasik please copy the description from original PR, so wouldn't have to click dozen of links to get needed information. for example, I'd like to know why this is BC break.

[samsonasik]

@glensc updated, thanks ;)

[glensc]

@samsonasik info why this is BC break still missing.

[samsonasik]

The removing of public method is BC break, that is in the description.

[glensc]

@samsonasik write it so then

[samsonasik]

that is in the description that I copied

[glensc]

@samsonasik seems you didn't get my message saying that copy text from the original issue, so wouldn't have to click dozen of links to get the needed information. for example, I'd like to know why this is BC break. now that info is spread around in comments in several issues/pull requests, not in compact form for current and future readers can find in one place.

[weierophinney]

This part is the BC break; users calling those methods now will have errors occur.

There are two options:

• Bump to a new major version. This is not terribly enticing, as new major versions often take time to get traction, and we'd likely want to delay release until there are other BC breaks in place that we want to achieve.
• Keep the methods, but change them to indicate deprecation, as well as to modify how they work slightly.

If we went the second route:

• We can remove the parameter typehint on setPluginClassLoader() (type widening is allowed for parameters), and internally check for either a HeaderLoader instance OR a laminas-loader PluginClassLocator. In the second case, we'd raise a deprecation notice.
• We'd change the return typehint on getPluginClassLoader() to HeaderLoader|PluginClassLocator, and raise a deprecation notice if it is called.

We'd need to ensure that the same property name is used by both these methods and resolveHeaderClass().

[samsonasik]

@weierophinney I've updated keep setPluginClassLoader() and getPluginClassLoader() with E_USER_DEPRECATION mark and keep property pluginClassLoader

[weierophinney]

@samsonasik I pushed additional changes to your branch. Basically:

• Added accessors for the header loader. This sort of extensibility is necessary so that users can accommodate custom header types. By providing new accessors, we detach them from the naming of the old implementation.

• Modified resolveHeaderClass to call on the new getHeaderLoader() to do lazy loading.

• Reverted a removed test for multi-part header lines.

• Added tests to ensure that the deprecation notices are being emitted.

• Added tests for the new accessors.

I think it's ready to ship now!

[weierophinney]

@samsonasik I almost merged this, and then realized there's a bigger BC break we need to address.

Removing the extends PluginClassLoader from HeaderLoader without implementing the public methods it defined is the big BC break here. With it removed, users who previously interacted with the default instance now will have breakage.

What we need to do is implement these methods:

• public static function addStaticMap($map)
• public function registerPlugin($shortName, $className) (this is the add() method you defined)
• public function registerPlugins($map)
• public function unregisterPlugin($shortName) (this is the remove() method you defined)
• public function getRegisteredPlugins()
• public function isLoaded($name) (this is the has() method you defined)
• public function getClassName($name)
• public function load($name) (this is the get() method you defined)
• public function getIterator() (you'll need to implement IteratorAggregate)

We can keep your existing names and mark the old ones as deprecated if you'd like; the main thing is ensuring that the API is 1:1 with what we had previously.

I can likely work on this Wednesday 2020-07-29 if you don't have time.

[glensc]

@weierophinney how do you trigger autoloader then? via autoload.files?

• https://getcomposer.org/doc/04-schema.md#files

[weierophinney]

@glensc —

I think you should do Symfony-style silent deprecation messages

That's a cool technique; I'd not seen using @ with trigger_error before. I'll update the code accordingly.

how do you trigger autoloader then? via autoload.files?

Yes. Generally, we've created an autoload.php file that checks to see if the class exists, and, if not, it then loads the file defining it. In this case, we'd need to provide both the interface and the implementation.

However, I don't think it's going to happen right now. More below.

---

@samsonasik and @michalbundyra —

I've been thinking on this a ton, and there's no clean way to do this in a BC manner. However, we can make a change that provides forwards compatibility and formally deprecates usage of laminas-loader, so that a v3 can remove it.

Here's how.

• We leave HeaderLoader unchanged.

• We create a new interface, HeaderLocatorInterface, that defines a resolveHeaderClass(string $headerName): string method.

• We create a new class, HeaderLocator, that implements this new interface and provides essentiallly the same API as you have proposed for HeaderLoader, to make it easy to add custom HeaderInterface implementations to the mix.

• The method setHeaderLocator() is modified to accept the new interface, and the getHeaderLocator() method is modified to return the new interface, lazy-loading the new HeaderLocator implementation when none is provided. They each work with a new $headerLocator property.

• Internally, resolveHeaderClass():

  • Checks if $pluginClassLoader is null. If not, it uses that to load the requested header.
  • If $pluginClassLoader is null, it calls getHeaderLocator() to retrieve the HeaderLocator instance, and then the value of calling its get() method.

We then document in both the CHANGELOG and the documentation the new interface, implementation, and default behavior, so that users know that they need to start modifying their code if they relied on the PluginClassLocator implementation.

Thoughts?

[samsonasik]

Laminas\Loader\PluginClassLocator can be changed with PluginClassLocator as the class re-added to use statement.

[samsonasik]

@weierophinney I think that HeaderLocator will be ok, but that means laminas-loader will only in require-dev? or still in require ?

[weierophinney]

I think that HeaderLocator will be ok, but that means laminas-loader will only in require-dev? or still in require ?

Still in require until we can remove it in 3.0. Essentially, I'm proposing we change the scope of this PR to make a forwards-compatible change so we can deprecate the usage of laminas-loader, since we have (a) a class that extends a class in that package, and (b) methods that are tied to an interface from that package, and (c) the latter are the default way to manipulate what custom headers can be handled by end-users. The PR would offer another way of doing this that is not tied to an external package, allowing us to deprecate those features, and then remove them in 3.0.

I'm willing to do the work, if you'd like.

[samsonasik]

@weierophinney ok, I will let you continue. Thank you

[glensc]

you can use composer "replace" trick to remove "laminas-loader" from your installed vendors:

• https://github.com/glensc/eventum/blob/d94105835b32af5750592cb70cbcb8fa3c9f9252/composer.json#L90-L91

but this solution is suitable only for applications. don't do this in libraries or your users will hate you.

[weierophinney]

@michalbundyra and/or @samsonasik — changes are in place. I've updated the description to better detail what has finally happened, and detailed it in the changelog as well. Tests are green. :)

[weierophinney]

These are why I lumped the bump in PHP version into the patch: scalar type hints, return type hints, nullable types, and void return types. Essentially it is this patch that necessitates the bump in minimum version, which is exactly what the TSC decision was about in that first meeting. 😄

[weierophinney]

As @glensc pointed out, this is how Symfony does deprecation notices. They'll get caught in error handlers, but not if unhandled. This prevents breakage in most use cases, but if you log errors, you'll catch them in your logs.

[weierophinney]

This bit was interesting. Because I turned on strict types in this file, I discovered a test that was passing null to this method, which was incorrect. As such, I added this guard (and updated the test!).

[glensc]

@weierophinney can you add a summary to users what they should do to stop using zend-loader?

just upgrade and add composer "replace" to loader package?

• Fixes #39 Fixes #40 remove laminas-loader dependency #95 (comment)

assuming just used defaults previously

[weierophinney]

@glensc By default, if you are NOT calling getPluginClassLoader() or setPluginClassLoader(), Headers will no longer use the laminas-loader functionality. This is spelled out in the changelog:

The value of getHeaderLocator() will now be used as the default mechanism for resolving header names to header classes.

Beyond that, if you're using the laminas-mvc, you cannot really replace the laminas-loader package, as it's used by the MVC (though the usage is largely deprecated). As such, I haven't detailed how to do that; it's a niche use case. But what it does do is open the door for us to remove it from this component in v3, and, if v3 is not released before releases of MVC components that do not use it, we can replace it in the laminas-mvc-skeleton package.