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
do not load duplicate command files, regardless of cache #430
Conversation
this is messy: we should probably remove the other check, but I am lost in this tangle and this works for me.
|
Looks about right, but I have not tried it. Also, I did not see any specific steps to reproduce in any of the referenced issues -- but perhaps I just didn't look hard enough. Could you provide a test, or aforementioned instructions? |
|
Code looks fine to be. "Remove the other check" probably refers to the $evaluated variable? That does look like a dupe of $loaded so we should indeed clean this up before committing. Does Aegir still use modulename.drush.load.inc files? If not, I'd like to remove that code. |
|
Nothing I have is using drush.load.inc files, and am fine with removing them if there is no identified critical use. |
|
Yes, Aegir use .drush.load.inc files quite a bit. We keep a registry of front-end (hosting) modules enabled, and use hook_drush_load() in the corresponding backend (provision) extensions. Moshe mentioned '--ignored-modules' on IRC, so I'll look into it as an alternative for Aegir 3. |
|
can we keep the .drush.load.inc issue separate? i believe we still need some similar mechanism, but this is beside the point here. |
|
i'll work on removing the redundant $evaluated check now. |
|
@weitzman how does that look now? |
|
I'm seeing warnings. Lets get to a green on the test suite and get this in. |
|
I'm having trouble running the test suite here, for some reason - care to share where the warnings are? |
|
siteUpgradeCase belongs to site-upgrade which was removed from Drush a long time ago. I suspect your Drush checkout is old or dirty. |
|
On 2014-02-07 15:10:43, Moshe Weitzman wrote:
Thanks, fixed. Running the tests again. (A long time ago... well that's just me, i'm getting old! ;) |
|
@anarcat - any progress on this? |
|
sorry, i didn't have time to followup on this... still need to run the tests... |
|
still haven't had time to test this here, but others confirmed this as working in #280. also, any idea why travis didn't pick this PR for testing? |
|
no idea why travis skipped this. i cant seem to find a way to get travis to test it again. maybe if you commit to this PR. |
| require_once $filepath; | ||
| unset($deferred[$module]); | ||
| $module = basename($filename); | ||
| $module = str_replace(array('.drush.inc', ".drush$dmv.inc"), '', $module); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$dmv isn't always defined, causing:
Undefined variable: dmv command.inc:1516
|
I suspect that this bug is improved (but not eliminated) by efdd5ac. That commit was backported to 6.x as well. Can you retry with latest code and see if the issue has gone away? |
|
I fixed anoth bug related to loading commandfiles from disabled modules under /profiles. Please try latest 6.x and master. Closing due to inactivity. |
this is messy: we should probably remove the other check, but I am
lost in this tangle and this works for me.
this should fix #280.