search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

function _installer_browser_is_project_enabled always returns false for layouts and always true for themes. #2336

Open klonos opened 8 years ago

klonos commented 8 years ago

https://github.com/backdrop/backdrop/blob/1.x/core/modules/installer/installer.browser.inc#L18

Discovered that while putting together a PR for #2335

PS: this is similar/related to #1709

PR by @klonos: https://github.com/backdrop/backdrop/pull/1644 PR by @klonos and @quicksketch: https://github.com/backdrop/backdrop/pull/1728

klonos commented 8 years ago

...while at it, I also removed a stray dpm from layout.admin.inc 😉

klonos commented 8 years ago

...turns out the logic in the same function for the themes is not correct either.

klonos commented 7 years ago

The PR passes for php70, but fails for php53. Not sure if the failures are random and would love some feedback as to what I may be doing wrong.

quicksketch commented 7 years ago

The PR passes for php70, but fails for php53.

Yeah, there's an issue with the use of the empty() function, which works slightly differently on newer versions of PHP. See https://github.com/backdrop/backdrop/pull/1644#pullrequestreview-15633495

I filed a PR fixing this issue at https://github.com/backdrop/backdrop/pull/1728

Thanks for finding that stray dpm() as well!

quicksketch commented 7 years ago

I've merged https://github.com/backdrop/backdrop/pull/1728 into 1.x and 1.5.x.

klonos commented 7 years ago

Actually, this still does not work for layouts...

klonos commented 7 years ago

...to reproduce the issue:

With the commit in this issue, this code has changed:

    case 'layout':
      $excluded_templates = config_get('layout.settings', 'excluded_templates');
      return layout_get_layout_template_info($name) && !in_array($name, $excluded_templates);
      break;

...the thing is that the $name variable passed to the _installer_browser_is_project_enabled function is a layout project name while layout_get_layout_template_info() returns the layout template names (thus layout_get_layout_template_info($name) will always return FALSE for layouts that include more than one templates and thus template name != project name). Same goes for $excluded_templates which also returns an array of layout template names. So the $name variable will never be in the $excluded_templates array for layouts that include more than one templates.

On top of that, since we are using layout_get_layout_template_info(), we need to first fix #984 and then pass the $rebuild argument to it inside this function.

jenlampton commented 7 years ago

Pushing this to the 1.6.2 milestone since the PR needs work and we are releasing 1.6.1 tonight.

docwilmot commented 4 years ago

Can we get this finished @klonos ? Seems like a simple cleanup, and has been sitting for way too long.

ghost commented 4 years ago

Since it's been a few years since any work was done here, I'm removing the milestone and adding back the candidate label. Once there's an RTBC PR, we can add the milestone back.

jenlampton commented 4 years ago

Can you check the criteria for bug-fix milestones? IIRC the PR does not need to be RTBC.

On Tue, Sep 22, 2020, 2:53 AM Peter Anderson notifications@github.com wrote:

Since it's been a few years since any work was done here, I'm removing the milestone and adding back the candidate label. Once there's an RTBC PR, we can add the milestone back.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/2336#issuecomment-696621170, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBERZNNXROZQ6NQMDN7OTSHBXYZANCNFSM4CWC55OA .

ghost commented 4 years ago

OK, maybe not exactly RTBC, but at least in a near-ready state. To me, two years with 'needs work' is not near-ready... I just think its ridiculous to have people needing to change the milestone every release for an issue that's clearly not a priority for anyone currently...

jenlampton commented 4 years ago

I just think its ridiculous to have people needing to change the milestone every release for an issue that's clearly not a priority for anyone currently...

It's no trouble, there's a single bulk-aciton to move all issues at once. :) One issue or 100 issues, it's all the same to us.

ghost commented 2 years ago

@jenlampton Sure, but it also adds unnecessary noise to the issue. I had to "Show hidden items" twice to see everything in this issue.

Also, removing PR labels since there's no PR (AFAIK) for the changes since the last PR was merged...