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

Drop dependency on zendframework/zend-loader #186

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

Drop dependency on zendframework/zend-loader #186

wants to merge 9 commits into from

Conversation

acelaya
Copy link

@acelaya acelaya commented Dec 21, 2017

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. Since they are public, this might be a BC break. What do you think?

@Xerkus Xerkus changed the base branch from master to develop December 21, 2017 15:17
@acelaya
Copy link
Author

acelaya commented Dec 21, 2017

Builds are failing with latest dependencies, but they are failing in master too.

I'll try to figure out how to fix them, but it might take me time.

@Xerkus
Copy link
Member

Xerkus commented Dec 21, 2017

Please rebase onto develop

@Xerkus
Copy link
Member

Xerkus commented Dec 21, 2017

There is a PR with fix, I will merge it shortly

@acelaya
Copy link
Author

acelaya commented Dec 21, 2017

Perfect then. Thanks.

@acelaya
Copy link
Author

acelaya commented Dec 21, 2017

I have rebased develop. I'll do it again once you merge the other PR.

@Xerkus
Copy link
Member

Xerkus commented Dec 21, 2017

Just a heads up: there is a big ongoing effort going for next major versions for zend-expressive and related packages as well as for zend-mvc. This change will require a next major version, and I do not expect to get to it until february.

@acelaya
Copy link
Author

acelaya commented Dec 21, 2017

Ok, no rush :-)

src/Headers.php Outdated
* @var \Zend\Loader\PluginClassLoader
*/
protected $pluginClassLoader = null;
const HEADERS_CLASS_MAP = [
Copy link
Member

Choose a reason for hiding this comment

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

This is not configurable.
Please move this into a variable in separate class with methods get($name), has($name), add($name, $class), remove($name)

Copy link
Author

Choose a reason for hiding this comment

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

I didn't realize it had to be configurable. I'll fix it right away.

Copy link
Author

Choose a reason for hiding this comment

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

If we move the logic to a class, should it be handled as the HeaderLoader used to?

Do we want to keep exposing it via public getter?

@Ocramius Ocramius added this to the 3.0.0 milestone Mar 1, 2018
xgin added a commit to plesk/zend-mail that referenced this pull request Nov 20, 2019
xgin added a commit to plesk/zend-mail that referenced this pull request Nov 20, 2019
xgin added a commit to plesk/zend-mail that referenced this pull request Nov 20, 2019
xgin added a commit to plesk/zend-mail that referenced this pull request Nov 20, 2019
@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-mail. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-mail to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-mail.
  • In your clone of laminas/laminas-mail, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra
Copy link
Member

This repository has been closed and moved to laminas/laminas-mail; a new issue has been opened at laminas/laminas-mail#39.

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.

None yet

6 participants