IBX-3957: Made NOP URL aliases not reusable and original

[barw4]

Question	Answer
JIRA issue	IBX-3957
Type	bug
Target Ibexa version	v3.3
BC breaks	?

Reasoning: because nop aliases are not original and they are reusable creating another alias with the same name will break such an alias, however, imo it should be incremented instead. This will prevent creating exactly the same alias with the same parent and as a result, breaking the old one.

Checklist:

• Provided PR description.
• Tested the solution manually.
• Provided automated test coverage.
• Checked that target branch is set correctly (master for features, the oldest supported for bugs).
• Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
• Asked for a review (ping @ezsystems/engineering-team).

[alongosz]

@barw4 as discussed internally, please add some integration coverage for the case this issue solves so I can analyse this further. At this point I'm not sure what would be the side effects here. Looks like we're just missing some coverage.

[barw4]

@alongosz an integration test is added, please see 2b9b4b7

[alongosz]

So now I know what's happening here. The end result is unfortunately the opposite of what the PR describes. A reusabe entry is the same entry either used for multiple purposes or that was repurposed. The changes you made here made effectively NOP entries NOT reusable. They're left behind as-is and the secondary entry gets created and named C2 according to the unique naming URL Alias logic.

The flow after the changes:

1. /a/b -> normal URL alias, a and b corresponding content items exist
2. /c/b -> custom URL alias, c is NOP because there's no c corresponding content
3. Rename a to c
4. /c2 now points to renamed content, /c is still a NOP entry therefore it wasn't reused.

What we'd want if we wanted to keep re-usability logic would be:

3. Rename a to c
4. /c/b becomes normal URL alias since c corresponds now to proper content node
5. /a/b -> is a historical entry redirecting to /c/b.

The issue with that is that renaming a to c causes custom URL alias being overridden and incorrectly cleaned up.

 id | parent | text |  action   | alias_redirects | is_alias | is_original | link 
----+--------+------+-----------+-----------------+----------+-------------+------
 43 |     42 | b    | eznode:63 |               0 |        0 |           1 |   43
 45 |     44 | b    | eznode:63 |               0 |        1 |           1 |   45
 42 |      0 | c    | eznode:62 |               1 |        0 |           1 |   42
 46 |      0 | a    | eznode:62 |               0 |        0 |           0 |   42

Notice that id=44 referenced in parent column is missing. It's supposed to be cleaned up via \eZ\Publish\Core\Persistence\Legacy\Content\UrlAlias\Gateway::cleanupAfterPublish but that neven happens because it's an edge case not considered before.

---

That being said, maybe your change is good, but what we're doing is actually making NOP not reusable, which is behavior change. Going with the fix for cleanup, even if possible, might lead to another issue - I'm not sure how system is going to behave if custom /c/b points to completely different Location. Then making it not reusable would make more sense.

Maybe this is something we can discuss with @adamwojs and @ezsystems/php-dev-team before we decide what direction we should take? Or is it clear for everyone and we can move forward with @barw4's fix and just change the wording?

[barw4]

This is obviously a naming mistake, you are correct. I relied too much on that comment above the if (https://github.com/ezsystems/ezplatform-kernel/pull/385/files#diff-d640f4389daf07a7bd0a474b4181c7b97c2c3208d172269c69fa59151e1fe014L232) and confused myself. My intention was that an alias would get incremented with uniqueCounter++ and I therefore treated it as 'reusable', because it gets incremented (reused, right?😺). Sorry about that

[barw4]

Test improved in bd4088e

[alongosz]

No problem, URL alias nomenclature was never easy or straightforward ;)
Now we need to decide if that's the change that we really want. Will discuss this with the Team and probably Adam.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[alongosz]

This is ok now, but I need to talk to Adam to make final decision, before giving approval.

[alongosz]

@adamwojs @webhdx we need to sync on that, there's behavior change here.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[barw4]

Closing in favor of ibexa/core#350 (v5)