Skip to content

Fix viewing unattached domain records #19545

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

Open
wants to merge 1 commit into
base: 10.0/bugfixes
Choose a base branch
from

Conversation

cconard96
Copy link
Contributor

Checklist before requesting a review

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.

Description

It seems like having domain records not linked to a domain was an intentional design since there is even a UI feature to link unattached records to domains. However, domain records use a CommonDBChild class which by default requires they be attached to a parent item (domain). While you could create records without a domain (probably another bug, but I don't want to dive into that right now), you would not be able to view them because it would fail a permission check. When canChildItem can't find a parent item, it will return false if it must be attached.

@cedric-anne
Copy link
Member

It seems like having domain records not linked to a domain was an intentional design since there is even a UI feature to link unattached records to domains.

Could you give a link to the corresponding code ?

@cconard96
Copy link
Contributor Author

It seems like having domain records not linked to a domain was an intentional design since there is even a UI feature to link unattached records to domains.

Could you give a link to the corresponding code ?

Domain records explicitly inherit entity info from domains but only if they are linked:
https://github.com/glpi-project/glpi/blame/0c6f0c527ba7194ce3fe58d6062fc6bb68d7270a/src/DomainRecord.php#L276-L280

Domain dropdown in record form doesn't have the empty option hidden:
https://github.com/glpi-project/glpi/blame/0c6f0c527ba7194ce3fe58d6062fc6bb68d7270a/src/DomainRecord.php#L357-L365

Domain dropdown in records tab to "Link a record" has SQL criteria to show records with a domains_id of <= 0 or null.
https://github.com/glpi-project/glpi/blame/0c6f0c527ba7194ce3fe58d6062fc6bb68d7270a/src/DomainRecord.php#L544-L555

@trasher
Copy link
Contributor

trasher commented Apr 29, 2025

It seems like having domain records not linked to a domain was an intentional design since there is even a UI feature to link unattached records to domains.

It's been a while since this has been added; but I do not remember we explicitely want records not linked to domain; I see ne case when it really makes sense.
Not sure at all it was intentional.

Domain dropdown in record form doesn't have the empty option hidden: https://github.com/glpi-project/glpi/blame/0c6f0c527ba7194ce3fe58d6062fc6bb68d7270a/src/DomainRecord.php#L357-L365

Domain dropdown in records tab to "Link a record" has SQL criteria to show records with a domains_id of <= 0 or null. https://github.com/glpi-project/glpi/blame/0c6f0c527ba7194ce3fe58d6062fc6bb68d7270a/src/DomainRecord.php#L544-L555

Therefore, those both seems bugs that should be fixed.

Current PR seems wrong to me; correct fix is probably to ensure records are all linked.

@cedric-anne
Copy link
Member

It seems like having domain records not linked to a domain was an intentional design since there is even a UI feature to link unattached records to domains.

It's been a while since this has been added; but I do not remember we explicitely want records not linked to domain; I see ne case when it really makes sense. Not sure at all it was intentional.

Domain dropdown in record form doesn't have the empty option hidden: https://github.com/glpi-project/glpi/blame/0c6f0c527ba7194ce3fe58d6062fc6bb68d7270a/src/DomainRecord.php#L357-L365
Domain dropdown in records tab to "Link a record" has SQL criteria to show records with a domains_id of <= 0 or null. https://github.com/glpi-project/glpi/blame/0c6f0c527ba7194ce3fe58d6062fc6bb68d7270a/src/DomainRecord.php#L544-L555

Therefore, those both seems bugs that should be fixed.

Current PR seems wrong to me; correct fix is probably to ensure records are all linked.

I agree. It does not make sense to have records not linked to a domain. Maybe it was a behaviour present in the domains we have preserved during the integration into GLPI.

@orthagh, what your opinion on this ?

@orthagh
Copy link
Contributor

orthagh commented Apr 29, 2025

None, really.
Just check the powerdns integration we maintain.
Otherwise, make the fix needed.

@trasher
Copy link
Contributor

trasher commented Apr 29, 2025

On PowerDNS side, records are depending on a domain.

@cconard96
Copy link
Contributor Author

So, same as #18360. We have a case where a child itemtype is supposed to always be linked but that wasn't previously designed like that. We cannot force users to fix orphan records, and therefore essentially have to keep supporting orphaned domain records except maybe preventing new orphans from being created.

@trasher
Copy link
Contributor

trasher commented May 5, 2025

#18360 is about transfer, that's not really the scope here (I'm almost sure there are still lot of issues in transfer process).

Domains record must be linked to a domain - that makes no sense to have standalone ones. If transfer allows that, that's a transfer bug. Current PR is still not acceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants