Problem/Motivation

Code in FormHelper classes seems unnecessarily confusing and complicated. This makes it difficult to add new features and fix bugs that affect these classes. Simplifying the code and implementing a clear structure will solve this problem.

Proposed resolution

Implement a clear mechanism for altering entity forms. Simplify code in FormHelper classes and move the form alter itself into separate classes.

User interface changes

No changes.

API changes

No changes to the public API (hooks, entities, plugins).

Data model changes

No changes.

Issue fork simple_sitemap-3271721

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:

About issue forks

Comments

WalkingDexter created an issue. See original summary.

WalkingDexter opened merge request !41

walkingdexter’s picture

walkingdexter commented
Assigned: walkingdexter » gbyte
Status: Active » Needs review

@gbyte I worked hard on this issue behind the scenes, please review the above changes. Maybe you have some additions or comments. Or maybe you disagree with the proposed implementation. Thanks!

gbyte’s picture

gbyte
Location Berlin
commented
Assigned: gbyte » walkingdexter
Status: Needs review » Needs work

Indeed this looks like a ton of work. I haven't had time for a full review yet, but I glanced through the code and I find it has improved a lot - very clean and well structured. What I did find is an exception thrown whenever I click on a 'Configure' Button on /admin/config/search/simplesitemap/entities - can you take a look?

walkingdexter’s picture

walkingdexter commented
Assigned: walkingdexter » gbyte
Status: Needs work » Needs review
gbyte’s picture

gbyte
Location Berlin
commented
Assigned: gbyte » walkingdexter
Status: Needs review » Needs work

Thanks for the fix; I just found some more time to test and found another bug:

When page bundle of the node type is enabled, saving the bundle on admin/structure/types/manage/page while IndexNow checkbox is checked makes the engines module want to send the bundle entity to IndexNow (which fails with an exception, as it does not have a canonical route).

Instead what should be happening: An IndexNow default should be set for entities of that bundle. 4.x does not have that problem yet.

walkingdexter’s picture

walkingdexter commented
Assigned: walkingdexter » gbyte
Status: Needs work » Needs review

Yep, my bad. The fix is ​​ready.

  • gbyte committed f97604b on 4.x authored by WalkingDexter 
    Issue #3271721 by WalkingDexter, gbyte: Refactor FormHelper classes
gbyte’s picture

gbyte
Location Berlin
commented
Assigned: gbyte » Unassigned
Status: Needs review » Fixed

Great work, thanks.

gbyte’s picture

gbyte
Location Berlin
commented

What happened to the tests?

walkingdexter’s picture

walkingdexter commented

This looks strange. Retest doesn't help. It seems that the problem is not in the module (according to the error description).

Status: Fixed » Closed (fixed)

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