In #3074998-5: Add explicit information about core compatibility to update data tedbow found that the core key became a requirement for 8.x long ago with #2188661: Extension System, Part II: ExtensionDiscovery.

Assuming there aren’t significant projects, themes and modules, working just as a side-effect of packaging, we can remove setting it in packaging, https://git.drupalcode.org/project/drupalorg/blob/dev/drupalorg_project/...

Comments

drumm created an issue. See original summary.

drumm credited tedbow.

drumm’s picture

drumm
he/him
Location NY, US
commented
Title: Remove core » Remove core key overriding in .info.yml packaging
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Priority: Normal » Major
Related issues: +#2313917: Core version key in module's .info.yml doesn't respect core semantic versioning

@drumm thanks for making this issue

  • Assuming there aren’t significant projects, themes and modules, working just as a side-effect of packaging, we can remove setting it in packaging,

    I think this is a safe assumption. #2188661-69: Extension System, Part II: ExtensionDiscovery was committed in February 2014 and Drupal 8.0.0 was released in November 2015.

    That means any developer that was actually creating or updating a module since Drupal 8 was released would run into this problem anytime they tried to enable a module or theme(via web or drush) or going to update.php. when a developer was working on their module/theme in git and i.e. not packaged from Drupal.org the core file has to be there.

    Also this is not just message that they could ignore this throws an exception which stops /admin/modules, admin/appearance, and update.php from loading and stops drush from enabling modules or themes.

    The only convienceable way you could have module or theme without a core file is if it has not been touched since Drupal 8.0-alpha10 was released in April 2014 or if the developer committed all changes to git without ever doing any testing either manual or via tests(I guess except unit).

  • Bumping this to major since #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning is marked as critical that has been confirmed by the 2 Drupal 8 release managers. If #2313917 gets committed and this change is not made then any module that attempts to use the new key
    core_dependency: ^8.8 || 9
    and intentionally does not provide a core key(the patch currently doesn't let them add a core key in this case) would have the core key added by Drupal.org packaging.

    Adding the core: key back would actually make the module compatible with versions of Drupal before 8.8.0 since they don't take the new core_dependency key into account.

drumm’s picture

drumm
he/him
Location NY, US
commented
Assigned: Unassigned » drumm

Probably overkill, but I’m checking out the 5,501 contrib 8.x project branches to see how many have a .info.yml file with a bad core value.

drumm’s picture

drumm
he/him
Location NY, US
commented

* 5,501 contrib 8.x project branches with non-zero reported usage

Good news, as far as I can tell, none of the .info.yml files with a core key have a value other than 8.x. Parsing was done with grep, not a real yaml parser.

There are a few missing a core key:

  • bec-8.x-1.x/bec.info.yml
  • botscout-8.x-1.x/botscout.info.yml
  • browserclass-8.x-1.x/browserclass.info.yml
  • business_core-8.x-3.x/modules/reactjs/modules/reactjs_block/react_bootstrap_block.info.yml
  • bxslider_block-8.x-1.x/bxslider_block.info.yml
  • commerce_liqpay-8.x-1.x/commerce_liqpay.info.yml
  • custom_panels_blocks-8.x-4.x/custom_panels_blocks.info.yml
  • dropcap_ckeditor-8.x-3.x/dropcap_ckeditor.info.yml
  • ercore-8.x-1.x/ercore.info.yml - opened #3075911: core key is necessary in .info.yml files
  • ercore-8.x-1.x/modules/ercore_j_honor/ercore_j_honor.info.yml
  • ercore-8.x-1.x/modules/ercore_institution/ercore_institution.info.yml
  • ercore-8.x-1.x/modules/ercore_collaboration/ercore_collaboration.info.yml
  • ercore-8.x-1.x/modules/ercore_summary_views/ercore_summary_views.info.yml
  • ercore-8.x-1.x/modules/ercore_presentation/ercore_presentation.info.yml
  • ercore-8.x-1.x/modules/ercore_user_profiles/ercore_user_profiles.info.yml
  • ercore-8.x-1.x/modules/ercore_event/ercore_event.info.yml
  • ercore-8.x-1.x/modules/ercore_publication/ercore_publication.info.yml
  • ercore-8.x-1.x/modules/ercore_proposal/ercore_proposal.info.yml
  • ercore-8.x-1.x/modules/ercore_patent/ercore_patent.info.yml
  • ercore-8.x-1.x/modules/ercore_core/ercore_core.info.yml
  • ercore-8.x-1.x/modules/ercore_other_product/ercore_other_product.info.yml
  • ercore-8.x-1.x/modules/ercore_program/ercore_program.info.yml
  • ercore-8.x-1.x/modules/ercore_bartik/ercore_bartik.info.yml
  • ercore-8.x-1.x/modules/ercore_admin/ercore_admin.info.yml
  • ercore-8.x-1.x/modules/ercore_j_highlight/ercore_j_highlight.info.yml
  • express-8.x-1.x/modules/custom/express_back_to_top/express_back_to_top.info.yml
  • form_mode_control-8.x-2.x/form_mode_control.info.yml
  • high_tax_terms-8.x-1.x/high_tax_terms.info.yml
  • likebtn-8.x-2.x/likebtn.info.yml
  • musicians-8.x-1.x/musicians.info.yml
  • persian_date-8.x-4.x/persian_date.info.yml
  • relatedbyterms-8.x-1.x/relatedbyterms.info.yml
  • role_log-8.x-1.x/role_log.info.yml
  • seaside_admin-8.x-1.x/seaside_admin.info.yml
  • seaside_admin_toolbar-8.x-1.x/seaside_admin_toolbar.info.yml
  • shamsi-8.x-2.x/shamsi.info.yml
  • simple_tweets-8.x-1.x/simple_tweets.info.yml
  • sm_dev_portal-8.x-1.x/themes/custom/sm_dev_portal_theme/sm_dev_portal_theme.info.yml
  • sticky_sharrre_bar-8.x-1.x/sticky_sharrre_bar.info.yml
  • tara-8.x-1.x/tara.info.yml
  • tooltip_ckeditor-8.x-1.x/tooltip_ckeditor.info.yml
  • video_embed_youku-8.x-1.x/video_embed_youku.info.yml

The ones I spot-checked often have the result of packaging checked in, like https://git.drupalcode.org/project/persian_date/blob/8.x-4.x/persian_dat..., or they removed it thinking it was unnecessary, https://git.drupalcode.org/project/form_mode_control/commit/5a56d8207d41...

Drush make also does .info file rewriting for D7, I do not know if it also carried on the tradition to D8 .info.yml files.

  • drumm committed 3a19daa on 7.x-3.x, dev 
    Issue #3075062 by drumm, tedbow: Remove core key overriding in .info.yml...
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

I checked few of these

  1. business_core-8.x-3.x/modules/reactjs/modules/reactjs_block/react_bootstrap_block.info.yml is actually an empty file so that would not cause the exception
  2. bec, high_tax_terms both do cause an exception /admin/modules when you use git clone.

Thoughts

  1. So 1 way could avoid breaking this modules' packaging on drupal.org would be check if they have new key added #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning and if they do don't add the core key. If they are using the new key then don't add core:.

    The problem is then we have really coordinate the time of core commit for #2313917 and the commit of this fix and the deployment to drupal.org.

    It may just be easier to file issue with these project since it seems like only 25 unique projects and some like business_core may just have empty info.yml files.

  2. @drumm I am correct in thinking this will only affect new releases. Any current release would already have been packaged and the core: would already have been added?

    If so it seems filing the issue would cover it. Because hopefully the developer would notice the new issue when making a new release.

drumm’s picture

drumm
he/him
Location NY, US
commented

It may just be easier to file issue with these project since it seems like only 25 unique projects and some like business_core may just have empty info.yml files.

Yes, I think that is best. I am going ahead and deploying simply removing this logic in packaging. Anything already-packaged will remain as-is.

Hopefully maintainers will correct this before their next tagged release. I don’t think making the logic more-complex to support these without commits to improve them is worthwhile.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Active » Fixed

This is now deployed.

https://www.drupal.org/project/saml_sp/releases/8.x-3.x-dev has saml_sp.info.yml packaged as

name: 'SAML Service Provider'
description: 'API module to validate SAML IdP (Identity Provider) credentials.'
core: 8.x
package: SAML
type: module
configure: saml_sp.config_sp_form

# Information added by Drupal.org packaging script on 2019-08-19
version: '8.x-3.0-beta3+5-dev'
project: 'saml_sp'
datestamp: 1566248885
drumm’s picture

drumm
he/him
Location NY, US
commented

I opened issues for each contrib project, except business_core. They all list this issue as the parent issue.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

@drumm thanks for resolving this issue so quickly and filing the issue with the contrib projects!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I opened issues for each contrib project, except business_core. They all list this issue as the parent issue.

🤯👏

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented

Is this still relevant for the child issues left?

drumm’s picture

drumm
he/him
Location NY, US
commented

There is nothing actionable here, the packaging change is fully done. The contrib projects would only have been working when running packaged releases; they were already not fully working for development using a Git clone even before this change. It would be great if the maintainers all made the required changes; but abandonment happens.