Problem/Motivation

.module files are only necessary when there are preprocess functions now that almost all hooks can be OOP

Steps to reproduce

Open file

Proposed resolution

  • entity_test_form_entity_test_form_validate() to Callbacks
  • entity_test_form_entity_test_form_validate_check() to Callbacks
  • entity_test_field_default_value() to Callbacks
  • _entity_test_record_hooks() to EntityTestHooks
  • Add return type to moved functions.

Will be done in separate issue:

Remaining tasks

None

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3495963

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

nikolay shapovalov created an issue. See original summary.

nikolay shapovalov’s picture

Issue summary: View changes

Looks like need to split this to several issues.

  • move entity_test_create_bundle(), entity_test_delete_bundle() to EntityTestHelper::createBundle
  • move entity_test_entity_types() to EntityTestHelper::createBundle, convert cont to enum.

nikolay shapovalov’s picture

Issue summary: View changes
nikolay shapovalov’s picture

Status: Needs work » Needs review
nikolay shapovalov’s picture

Assigned: nikolay shapovalov » Unassigned
Status: Needs review » Needs work
nikolay shapovalov’s picture

Status: Needs work » Needs review
nikolay shapovalov’s picture

Issue summary: View changes

Forget about entity_test_delete_bundle(). Update IS. Double checked, all methods from module are covered.

berdir’s picture

Status: Needs review » Postponed

Reviewed a bit, I'd suggest to postpone this on the two child issues, we won't be able to remove the .module file due to the BC layer anyway though.

nikolay shapovalov’s picture

Title: Move helpers in entity_test.module and delete it » Move helpers in entity_test.module

MR rebased

nicxvan’s picture

Status: Postponed » Needs work

The children are in this can be updated.

We can't remove the deprecated but we can make it easier in the future.

nikolay shapovalov’s picture

Status: Needs work » Needs review

MR updated

berdir’s picture

Status: Needs review » Reviewed & tested by the community

Verified that there's nothing left in entity_test.module except the two deprecated functions.

I'm not sold yet on the Callbacks class/pattern used here in general, but for this module, it's an existing pattern that's just used here too.

  • catch committed a798064d on 11.x
    Issue #3495963 by nikolay shapovalov, berdir: Move helpers in...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Yeah I don't love the callbacks either but also can't get overly concerned about them considering this is test modules.

joelpittet’s picture

A long shot but could these helpers be committed on D10 or how do we use them in contrib to support D10? I tried copying search_api in #3505316: Fix reliance on deprecated .MODULE.inc files for hooks inside og but found D10 didn't have them https://git.drupalcode.org/project/og/-/jobs/4465178

nicxvan’s picture

DeprecationHelper is the standard way https://www.drupal.org/node/3379306

joelpittet’s picture

@nicxvan What should I do when the class doesn't exist, yet like in this case in my test for this change #3495966: Deprecate and replace entity_test_create_bundle(), entity_test_delete_bundle()?

Call to static method createBundle() on an unknown class
Drupal\entity_test\EntityTestHelper.

berdir’s picture

@joelpittet: A function/method exists check for either the old or the new one, combined with a phpstan-ignore-next-line for the BC path.

FWIW, this is exactly why I think having phpstan set to not allow fail is a a problem, especially on next minor. This is a deprecation, an early warning that this will stop working in ~2 years. There is absolutely no reason to break CI now and disrupt contribution workflows. You can disable phpstan on next major or allow it to fail. See https://git.drupalcode.org/project/diff/-/blob/2.x/.gitlab-ci.yml?ref_ty....

If phpstan is used purely as a code quality tool it's one thing, and it's also different in custom projects where you only care about one specific core version, but considering that we also use phpstan as this early-warning-system for deprecations, it is IMHO just not a good fit. Maybe there could be separate jobs that filter things depending on whether or not it's a deprecation message. But that's getting way off topic.

joelpittet’s picture

@berdir, Thanks for your insight! I ended up just copying the helper method into the test class that was using it, bypassing both the original helper function and the missing helper class altogether. It might come back to bite me later, but that’s a future problem. 😉 If the phpstan on next major causes too much of this, I will do as you say and allow fails, though gotta feel the pain first.

berdir’s picture

Your decision, but I think that's the wrong call. Those helpers are only one side of the coin. The other side is \Drupal\entity_test\Hook\EntityTestHooks::entityBundleInfo(). If you only copy one side, then if the implementation changes then it will break. Those two are tightly coupled.

My suggestion isn't that complicated, it's essentially something like this:

            if (method_exists(Error::class, 'logException')) {
              Error::logException(\Drupal::logger('pathauto'), $e);
            }
            else {
              /* @phpstan-ignore-next-line */
              watchdog_exception('pathauto', $e);
            }

Status: Fixed » Closed (fixed)

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