Problem/Motivation

Node forms are in Drupal\node namespace instead of Drupal\node\Form. That is inconsistent, and I can guess this is the first place anyone would look for Form examples in core.

Proposed resolution

Move them to Drupal\node\Form.
Extend existing classes from new classes
Deprecate existing classes

Remaining tasks

Review.

User interface changes

None.

API changes

Drupal\node\NodeForm is at Drupal\node\Form\NodeForm
Drupal\node\NodeTypeForm is at Drupal\node\Form\NodeTypeForm

CommentFileSizeAuthor
#12 interdiff.txt987 bytessumitmadan
#12 move_node_forms_to-2391313-12.patch3.16 KBsumitmadan
#10 move_node_forms_to-2391313-10.patch2.52 KBsumitmadan
#4 2391313-4.patch2.51 KBrpayanm
move-node-form.patch2.54 KBpenyaskito

Issue fork drupal-2391313

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

Palashvijay4O’s picture

Palashvijay4O commented

Great ! Indeed a quick Fix .

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

Reviewed the code, applied the patch locally and ran tests, everything looks good. Looks ready to be commited.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll, +Needs issue summary update

To evaluate whether we should make this change during the beta, we need to update the issue summary according to https://www.drupal.org/core/beta-changes to explain what improvements it makes and what the disruption of the change would be. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

And it needs a reroll too.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2391313-4.patch2.51 KB

rerolled

brantwynn queued 4: 2391313-4.patch for re-testing.

disasm’s picture

disasm commented
Issue summary: View changes

Adding requested template for beta changes to issue summary.

disasm’s picture

disasm commented
Issue summary: View changes
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Issue tags: +Needs reroll
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Needs work
sumitmadan’s picture

sumitmadan commented
Status: Needs work » Needs review
StatusFileSize
new move_node_forms_to-2391313-10.patch2.52 KB

Re-rolled against new code.

Status: Needs review » Needs work

The last submitted patch, 10: move_node_forms_to-2391313-10.patch, failed testing.

sumitmadan’s picture

sumitmadan commented
Status: Needs work » Needs review
StatusFileSize
new move_node_forms_to-2391313-12.patch3.16 KB
new interdiff.txt987 bytes

Hope it should pass now.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -Quickfix, -Needs issue summary update, -Needs reroll

I'm not 100% sure that release managers will approve this change, but otherwise looks good. Let's RTBC so they can tell their views on it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Postponed

Discussed with @catch - we don't think this passes beta evaluation. The patch is correct but the disruption is not worth it. We can do this in 8.1 by deprecating the forms in Node and copying them to the correct place, and then making the form in Node extend the new forms in Node/Form

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

Sounds like a solid plan, thanks for caring!

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

acbramley’s picture

acbramley commented
Status: Postponed » Needs work

This is still applicable. Implementation details in #14

acbramley’s picture

acbramley commented
Issue summary: View changes

acbramley opened merge request !11765

acbramley’s picture

acbramley commented
Issue summary: View changes
Status: Needs work » Needs review
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Needs work
acbramley’s picture

acbramley commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Agree with the 1 open thread actually.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Thanks for also creating the change record!

quietone’s picture

quietone commented
Title: Move node forms to proper namespace (Drupal\node\Form) » Move node forms to standard namespace, Drupal\node\Form

I changed the titled. I applied the MR and search for node/src/NodeForm, node/src/Form/NodeForm and the old namespace and found no occurrences. And some tweaks to the change record.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

I think this could use https://www.drupal.org/node/3509577 rather than keeping the old class on the filesystem?

acbramley’s picture

acbramley commented

Awesome! Have added that and removed the classes. Tested manually and it works well.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

I think that might be the first time we've actually used the deprecation class loader, nice to see it working in practice. Moving this back to RTBC since it's a small change from the previous RTBC'd state, I'll still commit this (not 5 minutes after RTBCing though).

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

  • catch committed 49c6df3c on 11.x 
    Issue #2391313 by acbramley, sumitmadan, penyaskito, rpayanm, catch,...

catch closed merge request !11765

catch’s picture

catch
he/him
Primary language English
commented

Status: Fixed » Closed (fixed)

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