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

Use Module::Runtime instead of Module::Load #35

Closed
wants to merge 1 commit into from

Conversation

dolmen
Copy link
Member

@dolmen dolmen commented Nov 27, 2014

We just want to load a class from its package name. We don't need Module::Load's magic (the same function can load either files or packages). Also, Module::Runtime has workarounds for core perl bugs.

We just want to load a class from its package name. We don't need
Module::Load's magic. Also, Module::Runtime has workarounds for core
perl bugs.
@Leont
Copy link
Member

Leont commented Nov 27, 2014

Does this really solve any problem?

@karenetheridge
Copy link
Member

It does seem reasonable to me to use a less-magical solution here.

@dolmen
Copy link
Member Author

dolmen commented Nov 27, 2014

My general view is that we have many modules on CPAN that load modules dynamically (basically wrappers around require): Module::Runtime, Module::Load, Module::Pluggable, Class::Load... So many that each of them is a door open to already known or future security issues. Module::Runtime is the one I trust the most to have the right fixes. In a big application that uses many modules (such as dzil, see miyagawa/Dist-Milla#24) I also find particularly inefficient and unsafe to have many of those loader modules. That's two reasons why I have this new personal quest of unifying dynamic loading towards Module::Runtime.

I find Module::Load particularly flawed as it does too much magic. It is particularly insane to use it when we know that we will only load modules from their package name.

Does it fixes a known issue in Software::License? no.

@jandubois
Copy link
Member

I have this new personal quest of unifying dynamic loading towards Module::Runtime.
I find Module::Load particularly flawed as it does too much magic.

I find Module::Runtime flawed because it intentionally breaks CORE::GLOBAL::require overloading (https://rt.cpan.org/Public/Bug/Display.html?id=9565), so I don't think it is something to standardize on. :(

@karenetheridge
Copy link
Member

it intentionally breaks CORE::GLOBAL::require overloading (
https://rt.cpan.org/Public/Bug/Display.html?id=9565),

that's actually https://rt.cpan.org/Ticket/Display.html?id=95650

@karenetheridge
Copy link
Member

I find Module::Runtime flawed because it intentionally breaks CORE::GLOBAL::require overloading

It would be great if Zefram could comment on this issue, perhaps to explain why the override is ignored. Or, perhaps, someone just needs to submit a patch?

@jandubois
Copy link
Member

explain why the override is ignored

He does provide an explanation in the commit message you quote at https://rt.cpan.org/Public/Bug/Display.html?id=95650#txn-1363166.

It is basically: a CORE::GLOBAL::require override might throw an "failed to load module" error in format different from CORE::require. So if the heuristics in Module::Runtime doesn't parse this error string correctly, then it can't re-point the message at its own caller, and the error will point to a location within Module::Runtime, which wouldn't be helpful to the end-user.

So to avoid the risk of getting a less helpful error message, he disables the override mechanism altogether. Brings up thoughts of babys and bathwater...

Or, perhaps, someone just needs to submit a patch?

I don't see how you can patch against this risk. I would just argue that it is a risk you simply have to accept if you want to provide a shim around require().

@karenetheridge
Copy link
Member

I don't see how you can patch against this risk.

I mean, provide a patch that removes this disabling of the override. He may just apply the patch, if all the work is already done for him.

I think we need to find someone who is in communication with Zefram and can persuade him to look at this issue again. I'm not the right person; I'm already waiting for him to ship other fixes in other modules (namely Carp)...

@rjbs
Copy link
Member

rjbs commented Nov 29, 2014

For the record, the reason that S-L is using Module::Load is that it's trying to reduce non-core prereqs so it can be used closer to the toolchain without issues. There has been talk of bringing Module::Runtime into the core, but the things that jdb brought up are known problems with that.

@adamkennedy
Copy link
Member

I concur on the "core dependency beats better-but-equivalent dependency"
sentiment.

Adam
On Nov 29, 2014 5:23 AM, "Ricardo Signes" notifications@github.com wrote:

For the record, the reason that S-L is using Module::Load is that it's
trying to reduce non-core prereqs so it can be used closer to the toolchain
without issues. There has been talk of bringing Module::Runtime into the
core, but the things that jdb brought up are known problems with that.


Reply to this email directly or view it on GitHub
#35 (comment)
.

@rjbs rjbs closed this Dec 25, 2014
@karenetheridge karenetheridge deleted the dolmen/use-Module-Runtime branch June 9, 2017 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants