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.

Sign up for GitHub

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

Changes to Pathauto #21

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

Changes to Pathauto #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0fd0cac
Make sure jQuery selectors are inside context
Oct 2, 2014
9066b19
Modify if to else if
Oct 2, 2014
9e2ad6f
Removed unused PATHAUTO_IGNORE_WORDS constant in module file
Oct 2, 2014
28936ef
Removed taxonomy_get_tree in favor of the \Drupal::entityManager()
Oct 2, 2014
c609b6e
Removed duplicate ignore_words
Oct 3, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion config/install/pathauto.settings.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ max_length : 100
max_component_length: 100
transliterate : FALSE
reduce_ascii : FALSE
ignore_words : ', in, is,that, the , this, with, '
case : 1
ignore_words : 'a, an, as, at, before, but, by, for, from, is, in, into, like, of, off, on, onto, per, since, than, the, this, that, to, up, via, with'
update_action : 2
6 changes: 3 additions & 3 deletions pathauto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@
Drupal.behaviors.pathFieldsetSummaries = {
attach: function (context) {
$('fieldset.path-form', context).drupalSetSummary(function (context) {
var path = $('.form-item-path-alias input').val();
var automatic = $('.form-item-path-pathauto input').attr('checked');
var path = $('.form-item-path-alias input', context).val();
var automatic = $('.form-item-path-pathauto input', context).attr('checked');
Copy link

Choose a reason for hiding this comment

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

We should make sure to backport this change to the D7 version.

Copy link
Member

Choose a reason for hiding this comment

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

We should. This code is kind of pointless in D8, at least with seven, because the new seven UI doesn't display it anymore. But core still has it everywhere too and is displayed if you use the frontent theme, unless that replaces it too.

Copy link
Author

Choose a reason for hiding this comment

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

So for the D7 branch, should I post a patch in the pathauto issue queue? Would that be helpful?

Copy link

Choose a reason for hiding this comment

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

yep, I guess you can raise a issue with version 7.x-1.x-dev at https://www.drupal.org/node/add/project-issue/pathauto


if (automatic) {
return Drupal.t('Automatic alias');
}
if (path) {
else if (path) {
return Drupal.t('Alias: @alias', { '@alias': path });
}
else {
Expand Down
5 changes: 0 additions & 5 deletions pathauto.module
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,6 @@ use Drupal\Core\Routing\RouteMatchInterface;
use Drupal\node\Entity\Node;
use Drupal\user\Entity\User;

/**
* The default ignore word list.
*/
define('PATHAUTO_IGNORE_WORDS', 'a, an, as, at, before, but, by, for, from, is, in, into, like, of, off, on, onto, per, since, than, the, this, that, to, up, via, with');

/**
* Implements hook_hook_info().
*/
Expand Down
2 changes: 1 addition & 1 deletion src/PathautoManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ public function updateAlias(EntityInterface $entity, $op, array $options = array
* depending on the $load_entities parameter.
*/
protected function getTermTree($vid, $parent = 0, $max_depth = NULL, $load_entities = FALSE) {
return taxonomy_get_tree($vid, $parent, $max_depth, $load_entities);
return \Drupal::entityManager()->getStorage('taxonomy_term')->loadTree($vid, $parent, $max_depth, $load_entities);
Copy link
Member

Choose a reason for hiding this comment

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

The reason that was moved to a protected method is to be able to easily mock it in unit tests if we would add those.

We could possibly instead inject the entity manager in the constructor now and get the storage from that, then just do $this->termStorage->loadTree(). That is however not important, we want to remove the term specific logic from the pathauto manager anyway, so it is temporary.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, thanks for the info. If you want to remove the taxonomy specific stuff, I'd gladly help. Do you already have an idea where to put it? Maybe inside the module file in the hook_entity_update? Or should the module file in D8 serve more as a controller (and therefore be thin and only act as the glue code for services)?

Copy link
Author

Choose a reason for hiding this comment

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

I've looked at it more closely, and the module file is no good place at all because that would lead to code duplication on many places where PathautoManager::updateAlias is issued. I'm not sure what would be a good place though..

Copy link
Member

Choose a reason for hiding this comment

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

Don't worry about it for now. We're working on making the different pathauto pattern types plugins in #15, so we can possibly easily add a method somewhere in here or even move most of the logic here into a default method of a plugin base class.

}

/**
Expand Down