backdrop / backdrop-issues

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

"Undefined index: distribution_name" for Drupal migrations if profile is not "standard" #4799

Open ghost opened 3 years ago

ghost commented 3 years ago

If a Drupal site is upgraded to Backdrop and the install profile is not "standard" there are several php notices. This is because the installation doesn't run and 'distribution_name' doesn't get set. However when we get that information from the profile info file, we assume it's there and don't allow for instances where it doesn't exist. Then users get the following notice:

Notice: Undefined index: distribution_name in backdrop_install_profile_distribution_name() (line 222 of [SNIP]\core\includes\install.inc).

And also:

Notice: Undefined index: name in system_requirements() (line 33 of .../core/modules/system/system.install).
Notice: Undefined index: version in system_requirements() (line 35 of .../core/modules/system/system.install).

We should add a check to backdrop_install_profile_distribution_name() before attempting to return it.

ghost commented 3 years ago

PR: https://github.com/backdrop/backdrop/pull/3436

indigoxela commented 3 years ago

If a Drupal site is upgraded to Backdrop then the installation doesn't run and 'distribution_name' doesn't get set

This isn't the case for "normal" Drupal installs, I guess, because I wasn't able to reproduce it.

@BWPanda can you provide the steps to reproduce?

ghost commented 3 years ago

No, I didn't test it, just based my assumptions off the code and a report in the forum...

klonos commented 3 years ago

Edit: moved the testing steps over to #4801

No, I didn't test it ...

I'm hoping that we can refine the steps in #4801, so that we have easy to reuse steps to test vanilla D7 to Backdrop upgrades.

indigoxela commented 3 years ago

Another test, this time I could reproduce it, because I installed the minimal profile. My steps in detail:

Drupal:

Backdrop:

Final steps:

Notice: Undefined index: distribution_name still shows up on admin/modules/list Notice: Undefined index: name/version still shows up on admin/reports/status

Applied the PR patch

Notice: Undefined index: distribution_name is gone Notice: Undefined index: name/version still shows up on admin/reports/status

The remaining messages:

    Notice: Undefined index: name in system_requirements() (line 33 of .../core/modules/system/system.install).
    Notice: Undefined index: version in system_requirements() (line 35 of .../core/modules/system/system.install).

Besides the notices because it's not the standard profile, everything went smoothly.

indigoxela commented 3 years ago

@BWPanda I now reverted the patch and keep this testing web in that state to further test your PR after changes/updates.

Let's see how we make the notices go away. One down, two to go. :wink:

ghost commented 3 years ago

Thanks @indigoxela! Are those last two notices not part of https://github.com/backdrop/backdrop-issues/issues/2518? Maybe we should fix those there instead if so...

indigoxela commented 3 years ago

Are those last two notices not part of #2518?

Hm, good question, isn't this issue here a duplicate of that older one?

ghost commented 3 years ago

This is about the distribution name notice, that was about the other two notices.

indigoxela commented 3 years ago

This is about the distribution name notice

Sure, but the root cause for all three notices is the same: profile is not standard. The fact that distribution_name isn't mentioned over there might just have been an oversight.

Your current PR only suppresses the notice, but doesn't solve the base problem. I agree with that comment by @klonos. We really should handle this better, otherwise upgrading from D7 could cause unnecessary pain.

alanmels commented 3 years ago

Please note https://github.com/backdrop-contrib/profile_switcher module had been ported to Backdrop a while ago.

jenlampton commented 3 years ago

~The underlying problem here is that we're checking the list of modules for a profile, but we have type = profile in the profile's .info file so the profile doesn't qualify as a "module".~

Maybe not, in the system table the profiles sill register as modules... but it also shows minimal as disabled? Before the upgrade, in the Drupal 7 database minimal was enabled. I think we might be disabling profiles by accident, thinking they are modules.

jenlampton commented 3 years ago

Well, I just ran the upgrade again and it started enabled, and ended enabled. I don't know what's going on here. But once minimal was enabled I didn't get any of the undefined index issues.

ghost commented 2 years ago

I previously added a branch to the core repo with my PR changes, which was naughty. So I've deleted it. Need to start again with a branch in my personal fork (or someone else can take over).

klonos commented 2 years ago

I'm tentatively adding this to my to-do list ...unless someone else beats me to it.

klonos commented 1 year ago

Should we close this as a duplicate of #2518?