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

Refactor \Drupal\og_ui\BundleFormAlter #734

Open
wants to merge 15 commits into
base: 8.x-1.x
Choose a base branch
from

Conversation

pfrenssen
Copy link
Contributor

@pfrenssen pfrenssen commented Dec 18, 2021

The \Drupal\og_ui\BundleFormAlter class is in need of refactoring and bug fixing.

In #733 new scalar type hints are being introduced, and the testing of this has uncovered that the BundleFormAlter class is calling into Og::isGroup() with the $bundle_id set to NULL, while this method is only accepting strings.

Apart from this bug the class is also showing its age and is not up to date with current best practices. Let's:

  • Look into the architecture of the class. Now it is using a very workaround way of instantiating it with an entity with is derived from the form state, then later on the form and form state are passed and a protected ::prepare() method then acts as a second constructor which populates further properties. Internally then the form and form state are still passed on between methods. It looks like this can be simplified a lot, by just passing on the form and form state and using some basic helper methods. It looks like the whole class can operate without keeping any state, with purely static methods.
  • Introduce strict typing and type hints wherever possible.
  • Rename variables so that their purpose is clear and non-ambiguous.
  • Update tests, especially the AJAX functionality is not being tested in a proper way.

@pfrenssen
Copy link
Contributor Author

After spending some quality time with the class I am realizing it would better be converted into a service, since it depends on a bunch of other services such as the GroupTypeManager.

@pfrenssen pfrenssen self-assigned this Dec 19, 2021
@pfrenssen
Copy link
Contributor Author

This is ready but blocked by #732. Assigning to me to keep track.

@pfrenssen
Copy link
Contributor Author

Now that #732 has been merged, this is unblocked and ready for review. Unassigning.

@pfrenssen pfrenssen removed their assignment Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant