search

Perl-Toolchain-Gang / Software-License

perl representation of common software licenses
18 stars 40 forks source link

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

Closed dolmen closed 9 years ago

dolmen commented 9 years ago

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.

Leont commented 9 years ago

Does this really solve any problem?

karenetheridge commented 9 years ago

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

dolmen commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 https://github.com/Perl-Toolchain-Gang/Software-License/pull/35#issuecomment-64951669 .