Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#192 - Add autoload and extract Exception #411

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

#192 - Add autoload and extract Exception #411

wants to merge 1 commit into from

Conversation

codisart
Copy link

@codisart codisart commented Apr 3, 2018

I committed the vendor folder to keep the same installation process.
I added some tests to prove nothing is broken.

@mnewnham mnewnham added no change required core ADOdb core (library and base classes) and removed no change required labels Nov 26, 2019
@mnewnham mnewnham self-requested a review November 26, 2019 03:47
Copy link
Contributor

@mnewnham mnewnham left a comment

Choose a reason for hiding this comment

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

I wonder if you have any use cases / examples to share to explain how this might be implemented. For example if you it would be possible to implement without composer?

@codisart
Copy link
Author

codisart commented Dec 5, 2019

Hello @mnewnham,

This PR is a proposal to begin the work needed for #192 and #193.
I think it would be very hard to do this migration without using composer.

@mnewnham
Copy link
Contributor

mnewnham commented Dec 7, 2019

Although I was really adding a vague suggestion when I mentioned about not using composer, I would say that having to use it is probably a non-starter. You would get no push-back from me on it's greatness, but for those of us who work in large-scale, commercial software houses that depend on masses of legacy code interactions, integrating composer into them would be a complete nightmare. The other observation is that I've never come across a piece of software where the optimal implementation was composer that couldn't be installed with out it.

@dregad
Copy link
Member

dregad commented Jan 5, 2020

With regards to not using composer, for the time being (and foreseeable future too) we're still offering ADOdb for download and standalone installation, via the SourceForge downloads site.

I see no reason to force using Composer at all.

@codisart
Copy link
Author

codisart commented Jan 7, 2020

@dregad
Since I committed all files generated by composer, we still don't need composer to install ADOdb.
But I can see how it can be confusing.
I propose to simplify the autoloader generated by composer.
And maybe removing all mentions of composer.

@mnewnham
I'm sorry but I didn't understand what did you mean by :

The other observation is that I've never come across a piece of software where the optimal implementation was composer that couldn't be installed with out it.

Did you mean, that few libraries can only be installed via composer ?

@dregad
Copy link
Member

dregad commented Jan 9, 2020

maybe removing all mentions of composer

That would be too much, I think.

IMO, Composer would be the standard, default way to use the library in the future, but we need to provide a fallback way of using the library to accomodate usage scenarios like the one Mark described. This could indeed be a simplified autoloader script derived from the default Composer one.

@codisart
Copy link
Author

codisart commented Feb 20, 2020

Hello @dregad

I took some time to think about a solution to simplify the composer autoloader. But I must say that the composer code is quite complex and I did not find a good solution.
Furthermore I am not sure that this simplification is a good idea anymore.

By using the real code of composer, we are sure that no bug is introduced in the autoloading process. Except the ones from composer itself, but I think that a project as big as this one should be a good garanty of quality. The only downside of this current solution IMO is the autoloading will need to be updated to follow the composer history.

About the way the adobd project should be installed, my proposal changes nothing for now. And it allows to change this situation more easily in the future if it is needed.

What do you think ?

Edited:
It seems that I have removed my fork some times ago.
Should I recreate the PR ?

@gggeek
Copy link
Contributor

gggeek commented Jan 13, 2021

In case it might be of help: I faced a similar problem with the phpxmlrpc/phpxmlrpc package, which also dates to the dark ages of php 3. It is a smaller project than adodb in terms of classes and complexity of loading (ie. no drivers or plugins involved), but the basic issues were the same.

My approach:

  • rename existing classes, use proper namespaced class names and one-class-per-file (PRS-4)
  • allow young whippersnappers to use the lib via composer fullstop
  • keep the old .inc file available. in that file, classes are defined which extend the new classes while preserving the old class names
  • methods which you want renamed can be modified so in the new classes, and old-names methods kept in the .inc file, which call the new ones
  • the .inc file uses include_once calls to load the psr4 classes without going through a working autoloader config
  • add to the lib a custom autoloader which can be used by anyone wanting to use the psr4 classes but bypass composer

See:
https://github.com/gggeek/phpxmlrpc/blob/master/doc/api_changes_v4.md
https://github.com/gggeek/phpxmlrpc/blob/master/lib/xmlrpc.inc
https://github.com/gggeek/phpxmlrpc/blob/master/src/Autoloader.php

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core ADOdb core (library and base classes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants