Problem/Motivation

There are many examples throughout core and contrib, where some code wants to add some additional options, while keeping existing ones.
This can be cumbersome:

$query = $source_url->getOption('query');
$query['op'] = 'finish';
$query['id'] = $_batch['id'];
$source_url->setOption('query', $query);

Proposed resolution

What about adding some $url->mergeOptions() method to easy this process.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

Here is a test coverage as sort of a specification.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
1.77 KB
2.16 KB

Thanks for the spec test, that helped when deciding which way to merge.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Super cool. Thank you @tim.plunkett!

  1. +++ b/core/lib/Drupal/Core/Url.php
    @@ -675,6 +676,23 @@ public function setOption($name, $value) {
    +   * In the case of conflict with existing options, the new options will replace
    +   * the existing options.
    +   *
    

    +1 for this line of documentation.

  2. +++ b/core/lib/Drupal/Core/Url.php
    @@ -675,6 +676,23 @@ public function setOption($name, $value) {
    +    $this->options = NestedArray::mergeDeep($this->options, $options);
    

    Nice, to have this method being used!

The last submitted patch, 2: 2844001-2.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

The additional functionality should have a change record.

+++ b/core/tests/Drupal/Tests/Core/UrlTest.php
@@ -469,6 +469,29 @@ public function testGetOptions($urls) {
+    $url = Url::fromRoute('test_route', []);
+    $this->assertEquals([], $url->getOptions());
+    $url->setOptions(['foo' => 'bar']);
+    $this->assertEquals(['foo' => 'bar'], $url->getOptions());

Perhaps we want another called to ->setOptions() to prove is it destructive on the original options.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.25 KB
528 bytes

Perhaps we want another called to ->setOptions() to prove is it destructive on the original options.

Sure, why not.

Working on a CR now.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

There is a change record now.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b1500e0 and pushed to 8.3.x. Thanks!

My 7000th commit :)

  • alexpott committed b1500e0 on 8.3.x
    Issue #2844001 by dawehner, tim.plunkett: Add a Url::mergeOptions method
    

  • alexpott committed b1500e0 on 8.4.x
    Issue #2844001 by dawehner, tim.plunkett: Add a Url::mergeOptions method
    

  • alexpott committed b1500e0 on 8.4.x
    Issue #2844001 by dawehner, tim.plunkett: Add a Url::mergeOptions method
    

Status: Fixed » Closed (fixed)

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