Skip to content
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

[#393] Drupal 9 deprecations fixes #397

Merged
merged 16 commits into from
May 12, 2020

Conversation

arlina-espinoza
Copy link
Contributor

Makes apigee_edge module and submodules compatible with Drupal 9, as checked with the deprecation checker tool mglaman/drupal-check.

@googlebot googlebot added the cla: yes Indicates CLA has been signed label Apr 8, 2020
@arlina-espinoza arlina-espinoza linked an issue Apr 13, 2020 that may be closed by this pull request
@arlina-espinoza arlina-espinoza force-pushed the 393-d9-readiness branch 2 times, most recently from 10b512a to 987d25f Compare April 16, 2020 01:06
@mxr576
Copy link
Contributor

mxr576 commented Apr 21, 2020

@cnovak Is this PR going to create a new major version form the module? There are quite some changes on the public API or the module (like new menthods in interfaces, changed return types) that could break downstream projects that were using these.
(Removing deprecated code usages (which is a 👍 ) does not necessary require adding breaking changes to the codebase, see Drupal core.)

What is the strategy for supporting Drupal 8.x and Drupal 9.x when Drupal 9.0.0 it is out?

@arlina-espinoza arlina-espinoza marked this pull request as draft April 24, 2020 21:02
@arlina-espinoza arlina-espinoza force-pushed the 393-d9-readiness branch 3 times, most recently from d1a493a to 542c48a Compare April 27, 2020 05:48
@arlina-espinoza arlina-espinoza marked this pull request as ready for review April 27, 2020 06:36
@arlina-espinoza
Copy link
Contributor Author

@mxr576 We have discussed creating a new 2.x branch that supports both D8 and D9. However, I have updated the PR and I think there are no breaking changes any more.
Other notes:

  • The Apigee Edge Actions (rules integration) and submodules are not marked compatible with D9, as rules doesn't have a compatible release yet.

Testing
To test against the latest D9 beta, the key module needs this patch (https://www.drupal.org/project/key/issues/3116139). However, applying it using composer patches will still fail because composer says there is no available package of drupal/key compatible for D9. I forked the key module repo to make testing easier, and added it to the repositories section of the composer.json. In all. I'm using the following composer.json template to install and run D9 (it includes some fixes for other dev dependencies that were failing):
https://gist.github.com/arlina-espinoza/70515063318d23d88aabed2b18aac6c8

@arunz6161 arunz6161 added this to the 8.x-1.10 milestone Apr 28, 2020
@kscheirer
Copy link
Contributor

Instead of strtolower() I suggest using mb_strtolower() instead, which is multibyte safe. The change record is here: https://www.drupal.org/node/2850048.

@arlina-espinoza
Copy link
Contributor Author

Instead of strtolower() I suggest using mb_strtolower() instead, which is multibyte safe. The change record is here: https://www.drupal.org/node/2850048.

Good catch @kscheirer ! I have updated the PR to use multibyte functions where user input is processed. 👍

Copy link
Contributor

@shadcn shadcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Thanks @arlina-espinoza

Re. the singular label/lowercase deprecation, I think we should make the change so that label_singular annotation on the entity type is lowercase. This is how core does it: https://git.drupalcode.org/project/drupal/-/commit/13822906e5d7704a0df00dbd930b09a71f4b1972

See also \Drupal\Core\Entity\EntityTypeInterface::getSingularLabel

/**
   * Gets the indefinite singular form of the name of the entity type.
   *
   * This should return the human-readable name for a single instance of
   * the entity type. For example: "opportunity" (with the plural as
   * "opportunities"), "child" (with the plural as "children"), or "content
   * item" (with the plural as "content items").
   *
   * Think of it as an "in a full sentence, this is what we call this" label. As
   * a consequence, the English version is lowercase.
   *
   * @return string|\Drupal\Core\StringTranslation\TranslatableMarkup
   *   The singular label.
   */
  public function getSingularLabel();

@shadcn
Copy link
Contributor

shadcn commented May 12, 2020

@arlina-espinoza Thanks. LGTM.

@shadcn shadcn merged commit 73f1f3b into apigee:8.x-1.x May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates CLA has been signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drupal 9 readiness
6 participants